improve git package documentation (fix #231) #295

ajnavarro · 2017-02-28T14:10:30Z

No description provided.

codecov · 2017-02-28T14:21:53Z

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #295   +/-   ##
=======================================
  Coverage   77.22%   77.22%           
=======================================
  Files         117      117           
  Lines        7948     7948           
=======================================
  Hits         6138     6138           
  Misses       1159     1159           
  Partials      651      651

Impacted Files	Coverage Δ
references.go	`71.18% <ø> (ø)`	✅
remote.go	`69.61% <ø> (ø)`	✅
blame.go	`54.23% <ø> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 047a795...e408162. Read the comment docs.

smola · 2017-03-01T09:58:30Z

blame.go

 type BlameResult struct {
-	Path  string
-	Rev   plumbing.Hash
+	// Path is the path of the Blob object that we're blaming


We blame files, not blobs.

smola · 2017-03-01T09:59:53Z

blame.go

+	Path string
+	// Rev is the hash of the specified Commit used to generate this result
+	Rev plumbing.Hash
+	// Lines contains the last person that modifies this line, and the content itself


Not very clear. Lines contains lines, not person. Maybe reword:
Lines contains the last person that modifies this line, and the content itself -> Lines contrains every line with its authorship

smola · 2017-03-01T10:01:16Z

blame.go

@@ -98,8 +96,10 @@ func Blame(c *object.Commit, path string) (*BlameResult, error) {

 // Line values represent the contents and author of a line in BlamedResult values.
 type Line struct {
-	Author string // email address of the author of the line.
-	Text   string // original text of the line.
+	// email address of the author of the line.


Suggested reword: Author is the email address of the last author that modified the line

smola · 2017-03-01T10:01:47Z

blame.go

-	Text   string // original text of the line.
+	// email address of the author of the line.
+	Author string
+	// original text of the line.


Add Text at the begining, such as Text is the original text of the line

alcortesm · 2017-03-02T08:24:37Z

blame.go

@@ -13,17 +13,21 @@ import (
 	"srcd.works/go-git.v4/utils/diff"
 )

+// BlameResult represents the result of a Blame operation


Add a full stop at the end of the sentence, as it is going to be rendered as English prose by go-doc.

alcortesm · 2017-03-02T08:25:12Z

blame.go

 type BlameResult struct {
-	Path  string
-	Rev   plumbing.Hash
+	// Path is the path of the File that we're blaming


alcortesm · 2017-03-02T08:26:12Z

blame.go

-	Rev   plumbing.Hash
+	// Path is the path of the File that we're blaming
+	Path string
+	// Rev is the hash of the specified Commit used to generate this result


Full stop.

Maybe you should mention that rev is the abbreviation of revision, or change the field name to Revision.

alcortesm · 2017-03-02T08:26:17Z

blame.go

+	Path string
+	// Rev is the hash of the specified Commit used to generate this result
+	Rev plumbing.Hash
+	// Lines contains every line with its authorship


alcortesm · 2017-03-02T08:28:56Z

blame.go

-// The output is a slice of commits, one for each line in the file.
+// Blame returns a BlameResult that contains all the data needed to know the
+// last author of each line of an specified file starting the history from
+// a specified commit. The file to blame is identified by the input arguments:


I don't understand what you mean here by "starting the history from a specified commit".

The specified commit is the commit that you are using to call Blame. The BlameResult will change depending of the commit you are using.

Some alternative rewordings:

A) This is correct, but maybe too short and dependent on the BlameResult definition.

Blame returns a BlameResult for file `path` in commit `c`.

B) This one is more user friendly.

Blame returns a BlameResult with the information about the last author of each line from file `path` at commit `c`.

The original wording, written by me, was too verbose, because I was
not familiarized with go-git back then and I feel the need to explain
how the same file in different commits are different entries. Now I
clearly see being so verbose is no good.

I will also make private the rest of the paragraphs in this comment, as
you did with the one referencing the paper.

alcortesm · 2017-03-02T08:30:25Z

blame.go

-// described in Zimmermann, et al. "Mining Version Archives for
-// Co-changed Lines", in proceedings of the Mining Software
-// Repositories workshop, Shanghai, May 22-23, 2006.
-//


Why did you remove this comment? how are you going to understand the current implementation without the paper describing the algorithm? it is probably the most important comment in the whole file. I agree it should not be public, though, it should be private as it explains the implementation details.

Regarding to #231 (General questions) how the things are implemented is not documentation.

I will keep this comment inside the method.

alcortesm · 2017-03-02T08:31:36Z

blame.go

@@ -98,8 +96,10 @@ func Blame(c *object.Commit, path string) (*BlameResult, error) {

 // Line values represent the contents and author of a line in BlamedResult values.
 type Line struct {
-	Author string // email address of the author of the line.
-	Text   string // original text of the line.
+	// Author is the email address of the last author that modified the line


alcortesm · 2017-03-02T08:33:44Z

references.go

 // - Cherry-picks are not detected unless there are no commits between them and
-//   therefore can appear repeated in the list.
-//   (see git path-id for hints on how to fix this).
+// therefore can appear repeated in the list. (see git path-id for hints on how to fix this).


This comment line looks longer than the rest, it may be longer than 60 or 70 chars (or whatever the line length we are using for comments).

alcortesm · 2017-03-02T08:33:56Z

remote.go

@@ -32,7 +32,7 @@ func newRemote(s storage.Storer, c *config.RemoteConfig) *Remote {
 	return &Remote{s: s, c: c}
 }

-// Config return the config
+// Config returns the RemoteConfig object used to instantiate this Remote


alcortesm · 2017-03-02T11:19:13Z

blame.go

-// The output is a slice of commits, one for each line in the file.
+// Blame returns a BlameResult that contains all the data needed to know the
+// last author of each line of an specified file starting the history from
+// a specified commit. The file to blame is identified by the input arguments:


Some alternative rewordings:

A) This is correct, but maybe too short and dependent on the BlameResult definition.

Blame returns a BlameResult for file `path` in commit `c`.

B) This one is more user friendly.

Blame returns a BlameResult with the information about the last author of each line from file `path` at commit `c`.

The original wording, written by me, was too verbose, because I was
not familiarized with go-git back then and I feel the need to explain
how the same file in different commits are different entries. Now I
clearly see being so verbose is no good.

I will also make private the rest of the paragraphs in this comment, as
you did with the one referencing the paper.

mcuadros requested a review from alcortesm March 1, 2017 09:26

smola suggested changes Mar 1, 2017

View reviewed changes

smola approved these changes Mar 1, 2017

View reviewed changes

alcortesm suggested changes Mar 2, 2017

View reviewed changes

alcortesm approved these changes Mar 3, 2017

View reviewed changes

ajnavarro added 3 commits March 3, 2017 16:16

improve git package documentation (fix #231)

9006915

Add full stops and keep implementation details as internal comments.

be7e1e6

Simplify Blame documentation

e408162

ajnavarro merged commit 199317f into src-d:master Mar 3, 2017

ajnavarro deleted the improvement/package-git-doc branch March 3, 2017 15:51

improve git package documentation (fix #231) #295

improve git package documentation (fix #231) #295

Uh oh!

Conversation

ajnavarro commented Feb 28, 2017

Uh oh!

codecov bot commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Feb 28, 2017 •

edited

Loading

ajnavarro Mar 2, 2017 •

edited

Loading