Repo commits #49

ktoso · 2013-08-25T23:33:12Z

The long awaited commit with repo/commits API.
The API is different than git/commit in having more detailed information about the commit itself - for example lists of files, and authors etc.

The tests are a bit verbose. I've tested against the example jsons from the github api page, but that saved me from overlooking a field or two when updating the structs.

I've also moved this code to the new style that we've started using in the lib recently (pointers everywhere).
I skipped the URL fields as we did before, though not sure if maybe for the "compare commits" case the URL may be helpful - they're for patches etc.

Looking forward to your comments guys :)

Implements one of the APIs mentioned in #37

we have such code repreated in a few places

it also has a Stats field in the case of repos/commits

willnorris · 2013-08-26T15:15:35Z

github/repos_comments.go

@@ -13,7 +13,7 @@ import (
 // RepositoryComment represents a comment for a commit, file, or line in a repository.
 type RepositoryComment struct {
 	HTMLURL   *string    `json:"html_url,omitempty"`
-	URL       *string    `json:"url,omitempty"`
+	URL       *string    `json:"url,omitempty"` // todo: I thought we're skipping urls? - ktoso


yeah, I've basically given up on that. While I still don't think the URLs are terribly useful, I've stopped asking people to remove them. It's certainly simpler if our structs just include everything.

ok, removed comment then.

willnorris · 2013-08-26T15:55:18Z

A general note here about your tests... I intentionally don't include the full example from the GitHub docs. That's because it isn't really meaningful to do so because of what we're actually testing here. The portion of the test where we are parsing a sample response is doing two things: 1) verify that we can parse it into the expected data type and 2) verify that our json:"" tags on the fields are defined properly.

To satisfy number 1, the simplest possible response body will do, which is what most of our tests contain. This is primarily checking the JSON type of the outermost element of the response... making sure we're not trying to unmarshall a JSON object into a Go slice, for example.

Number 2 is not being fully covered in mosts of our tests right now (I talk about that in my recent blog post here). For that, we do need at least one test that includes all of the fields for a given type. It doesn't actually matter what the value is for all of those fields, because we're not actually testing for that. Therefore, I always use the shortest value possible (for example, for strings I use a single character, normally using the first letter of the field name). The shorter values makes tests easier to read, and makes it clear that we're not doing any validation on values.

Additionally, there is no need to include all of the fields for nested structs if there are other tests that cover them. For example, a Repository includes a nested User in it's owner field. Since we have tests for User, there's no value in re-testing all of the user fields again as part of our repository tests. Instead, we just include a single field so that the struct is included, and we can verify that it is indeed being unmarshalled into the right type. For nested structs that don't have separate tests (which you have a few of here, like CommitFile and others), then it does make sense to include all of the fields for the nested struct.

Additionally, if we're going to try and cover number 2 (which I think is certainly is a good thing) we only need to do it for a single test... any more than that is just duplicating effort with no additional benefit. I would prefer we do it on the "Get" method for each type, since that is the most discrete test for checking a single resource of the type.

So applying that logic results in:

for the "Get" method, include a full response body, using simple values for field values. For nested structs that have their own tests, include only a single field.
for all other tests, use very simple tests to verify that you can unmarshal into the expected type, but nothing else.

ktoso · 2013-09-04T00:10:30Z

Hey Will,
huge thanks for the tips and patience :-)

I've updated the sources according to the comments (no parameters.go, smaller tests - I'm testing new types introduced only in one place, etc.).

PS: Sorry about the weird delays in PR / fixes - I was in the middle of moving flats.

willnorris · 2013-09-04T18:17:32Z

hey, no worries... I just really appreciate you taking the time to do this. :) A few more inline comments forthcoming (mainly things I missed previously, I guess)...

willnorris · 2013-09-04T18:18:06Z

github/repos_commits.go

+// RepositoriesService.RepositoriesList method.
+type CommitsListOptions struct {
+	// Sha or branch to start listing commits from.
+	SHA *string


fields on Options structs don't need to be pointers... only data structures that are JSON encoded.

Hm... this Options struct is rather big, don't we have to use pointers to make some options "optional"? I can imagine people wanting to not specify Since for example.

I just noticed none of your tests pass a CommitsListOptions.. they all pass nil. I'm pretty sure that you will get an error trying to dereference these pointers if they are unset down around line 87, since you're not doing nil checks.

As far as I know, there are no cases in the GitHub API where the zero value is meaningful for these optional parameters. If there are, then of course we're going to run into the same problem that we have for data structures. But if not, then we can just leave them as non-pointer values, and let the zero values through. Today, they will be included in the query string, but with no value.... e.g. ?sha=&path=&author=, etc. I'm not actually sure what will happen with the since time. I think I typically check for !IsZero() before adding it to the url.Values. When I handle #50, we can easily strip all of these zero value params.

Right, that convinces me :-) For Times I have the IsZero checks so we're fine there. Moving away from pointers.

…e in tests

willnorris · 2013-09-04T21:58:37Z

github/repos_commits_test.go

+	}
+
+	// when
+	commits, _, err := client.Repositories.ListCommits("o", "r", &CommitsListOptions{


well you still gotta actually test the option values :) testFormValues makes this really simple... see https://github.com/google/go-github/blob/master/github/users_test.go#L94-L104

oh how nice, included it :) sorry and thanks again :)

ktoso · 2013-09-04T21:59:30Z

Should be nicely cleaned up now. Thanks a lot for the feedback :)

willnorris · 2013-09-04T23:32:09Z

merged (together with some other changes I forgot about) as 1eaf383

thanks for pushing through this one... this was definitely one of the hairier parts of the API in terms of data structures.

ktoso kind of shamed me with his extensive resource tests in #49 :), so I'm finally starting to setup a structure for more general testing of this type. This change adds the `testJSONMarshal` helper function, and adds tests for the `User` type. This includes two checks: - check that an empty resource produces an empty JSON object. This effectively verifies that all fields include 'omitempty' so that we're not unintentionally sending additional data over the wire. - check that a full resource with every field populated produces the expected JSON. This verifies that the JSON field mappings are correct. In this case, it might be okay to use resource samples from the GitHub docs, though I do still prefer very simple field values since it makes tests easier to read. When these tests are added for each resource type, we can reduce all of our other tests to return bare minimal response bodies, since the resource fields are already being tested at that point.

dmitshur · 2016-12-12T20:37:21Z

github/repos_commits.go

+	Author    *User    `json:"author,omitempty"`
+	Committer *User    `json:"committer,omitempty"`
+	Parents   []Commit `json:"parents,omitempty"`
+	Message   *string  `json:"message,omitempty"`


I suspect Message field was added here erroneously. It should only be inside Commit, not RepositoryCommit.

From what I can tell, it's not actually ever present in the GitHub API responses. Does anyone have counter-examples? If not, I'll make a PR to remove it. Edit: It has been done in #492.

SGTM. Thanks, @shurcooL.

While making that PR, I ran into another issue and submitted a fix at #491. That needs to be reviewed and merged first, @gmlewis.

This field is unused. There is no "commit message" inside of RepositoryCommit (higher level GitHub object), it's only inside of Commit (lower level git commit). It was added accidentally in #49 and not caught via test due to #491. Followup to #49. See https://github.com/google/go-github/pull/49/files#r92034462.

This field is unused. There is no "commit message" inside of RepositoryCommit (higher level GitHub object), it's only inside of Commit (lower level git object). It was added accidentally in #49 and not caught via test due to #491. Followup to #49. See https://github.com/google/go-github/pull/49/files#r92034462.

This field is unused. There is no "commit message" inside of RepositoryCommit (higher level GitHub object), it's only inside of Commit (lower level git object). It was added accidentally in google#49 and not caught via test due to google#491. Followup to google#49. See https://github.com/google/go-github/pull/49/files#r92034462.

ktoso added 4 commits August 26, 2013 01:00

try to extract optional GET params builder,

8249131

we have such code repreated in a few places

implement repos_commits API

5a82fcb

a Commit is a shared structure between git/commit and repos/commits API

ed198f0

it also has a Stats field in the case of repos/commits

typo in type fixed, it's "CommitsComparison"

3217b5e

willnorris reviewed Aug 26, 2013
View reviewed changes

ktoso added 2 commits September 4, 2013 01:01

simplifying the tests, removing parameters.go after code review

f7ab920

reuse existing type

8e70667

willnorris reviewed Sep 4, 2013
View reviewed changes

ktoso added 2 commits September 4, 2013 22:45

review followup, fix docs, remove helper func

7be56c9

review follow up, move away from * in Options and use it at least onc…

6bb8981

…e in tests

willnorris reviewed Sep 4, 2013
View reviewed changes

testing for proper Options, if the date format is OK + if all included

501b66f

willnorris closed this Sep 4, 2013

ktoso deleted the repo-commits branch September 5, 2013 20:00

ktoso restored the repo-commits branch September 5, 2013 20:01

ktoso deleted the repo-commits branch September 5, 2013 20:02

dmitshur reviewed Dec 12, 2016

View reviewed changes

dmitshur mentioned this pull request Dec 12, 2016

Fix broken test TestRepositoriesService_CompareCommits. #491

Closed

dmitshur mentioned this pull request Dec 12, 2016

Remove RepositoryCommit.Message field. #492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repo commits #49

Repo commits #49

ktoso commented Aug 25, 2013

willnorris Aug 26, 2013

ktoso Sep 4, 2013

willnorris commented Aug 26, 2013

ktoso commented Sep 4, 2013

willnorris commented Sep 4, 2013

willnorris Sep 4, 2013

ktoso Sep 4, 2013

willnorris Sep 4, 2013

ktoso Sep 4, 2013

willnorris Sep 4, 2013

ktoso Sep 4, 2013

ktoso commented Sep 4, 2013

willnorris commented Sep 4, 2013

dmitshur Dec 12, 2016 •

edited

Loading

gmlewis Dec 12, 2016

dmitshur Dec 12, 2016 •

edited

Loading

Repo commits #49

Repo commits #49

Conversation

ktoso commented Aug 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willnorris commented Aug 26, 2013

ktoso commented Sep 4, 2013

willnorris commented Sep 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Sep 4, 2013

willnorris commented Sep 4, 2013

dmitshur Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

dmitshur Dec 12, 2016 •

edited

Loading

dmitshur Dec 12, 2016 •

edited

Loading