Fixing issue with commit search results deserialization #601 #602

amirilovic · 2017-03-28T13:10:15Z

No description provided.

dmitshur · 2017-03-29T06:27:54Z

github/search.go

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


Based on #180 and #520 (comment) (/cc @gmlewis), I think we'd want to go with []*Commit here. Even though it's a little inconsistent with other similar structs.

At least that's what I think @gmlewis would request.

Got it, will fix.

gmlewis · 2017-03-29T17:05:01Z

As part of this cleanup, can you please add HTMLURL to type Commit struct in git_commits.go (I cam across this while looking at the parents response)?

Also, in search.go can you please rename Total in type CommitsSearchResult struct to TotalCount for consistency?

Since these are breaking API changes, we also need to bump the version number.

Thank you!

amirilovic · 2017-03-29T19:45:18Z

@gmlewis All fine for TotalCount.

For HTMLURL in Commit, I think it's fine right now.
If you take a look at https://developer.github.com/v3/search/#search-commits results are represented as CommitResult which does have HTMLURL

type CommitResult struct {
       ...
        Commit      *Commit  `json:"commit,omitempty"`
	HTMLURL     *string     `json:"html_url,omitempty"`
	....
}

Also for RepositoryCommit as a result of https://developer.github.com/v3/repos/commits/#get-a-single-commit

type RepositoryCommit struct {
	...
        Commit      *Commit  `json:"commit,omitempty"`
        HTMLURL     *string  `json:"html_url,omitempty"`
	...
}

So, as I understand both result structures are wrappers around a commit with a specific HTMLURL, this is inline with github docs.

Also parents in get single commit don't have html_url

...
"parents": [
    {
      "url": "https://api.github.com/repos/octocat/Hello-World/commits/6dcb09b5b57875f334f61aebed695e2e4193db5e",
      "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e"
    }
  ],
...

As far as I see in docs for other endpoints parents array is always just a reference to parent commits it never contains actual commit data, so maybe the best thing would be to create new struct to that represents that like:

type CommitReference {
        SHA *string  `json:"sha,omitempty"`
        URL *string  `json:"url,omitempty"`
        HTMLURL *string  `json:"html_url,omitempty"`
}

and change RepositoryResult and CommitResult to be:

type RepositoryCommit struct {
        ...
	Parents     []CommitReference `json:"parents,omitempty"`
        ...
}

type CommitResult struct {
        ...
	Parents     []CommitReference `json:"parents,omitempty"`
        ...
}

What do you think?

dmitshur · 2017-03-29T23:20:18Z

Also, in search.go can you please rename Total in type CommitsSearchResult struct to TotalCount for consistency?

@gmlewis, I don't think we should do this. At least not in this PR. It makes things inconsistent. Look at the rest of search.go:

// RepositoriesSearchResult represents the result of a repositories search.
type RepositoriesSearchResult struct {
	Total             *int         `json:"total_count,omitempty"`
	IncompleteResults *bool        `json:"incomplete_results,omitempty"`
	Repositories      []Repository `json:"items,omitempty"`
}

// IssuesSearchResult represents the result of an issues search.
type IssuesSearchResult struct {
	Total             *int    `json:"total_count,omitempty"`
	IncompleteResults *bool   `json:"incomplete_results,omitempty"`
	Issues            []Issue `json:"items,omitempty"`
}

// UsersSearchResult represents the result of a users search.
type UsersSearchResult struct {
	Total             *int   `json:"total_count,omitempty"`
	IncompleteResults *bool  `json:"incomplete_results,omitempty"`
	Users             []User `json:"items,omitempty"`
}

// CodeSearchResult represents the result of a code search.
type CodeSearchResult struct {
	Total             *int         `json:"total_count,omitempty"`
	IncompleteResults *bool        `json:"incomplete_results,omitempty"`
	CodeResults       []CodeResult `json:"items,omitempty"`
}

Do you see how it'd be inconsistent to change Total to TotalCount only in CommitsSearchResult?

// CommitsSearchResult represents the result of a commits search.
type CommitsSearchResult struct {
	TotalCount        *int            `json:"total_count,omitempty"`
	IncompleteResults *bool           `json:"incomplete_results,omitempty"`
	Commits           []*CommitResult `json:"items,omitempty"`
}

We've already discussed this at #520 (comment) previously, and you agreed to prefer consistency there.

@gmlewis Can you open an issue about this instead, so we can come to a decision and address all similar structs at once? But let's keep it out of scope of this PR.

@amirilovic Please revert TotalCount to Total for now.

gmlewis · 2017-03-29T23:28:31Z

OK, SGTM.
Opened: #605
Sorry for the trouble, @amirilovic.

dmitshur · 2017-03-29T23:41:42Z

About HTMLURL in Commit, I agree with @gmlewis that we should add it there.

@amirilovic, it's pretty subtle as to why.

If you look at our https://godoc.org/github.com/google/go-github/github#GitService.GetCommit method, it returns Commit. It maps to https://developer.github.com/v3/git/commits/#get-a-commit GitHub API endpoint, and its example output does not contain a "html_url" field:

{
  "sha": "7638417db6d59f3c431d3e1f261cc637155684cd",
  "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/7638417db6d59f3c431d3e1f261cc637155684cd",
  "author": {
    "date": "2014-11-07T22:01:45Z",
    "name": "Scott Chacon",
    "email": "schacon@gmail.com"
  },
  "committer": {
    "date": "2014-11-07T22:01:45Z",
    "name": "Scott Chacon",
    "email": "schacon@gmail.com"
  },
  "message": "added readme, because im a good github citizen",
  "tree": {
    "url": "https://api.github.com/repos/octocat/Hello-World/git/trees/691272480426f78a0138979dd3ce63b77f706feb",
    "sha": "691272480426f78a0138979dd3ce63b77f706feb"
  },
  "parents": [
    {
      "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/1acc419d4d6a9ce985db7be48c6349a0475975b5",
      "sha": "1acc419d4d6a9ce985db7be48c6349a0475975b5"
    }
  ]
}

However, GitHub API documentation is not always 100% up to date and does not always show all fields that the real API returns. This is an example of that.

If you hit that endpoint for real, it includes "html_url" field. For example, try:

https://api.github.com/repos/octocat/Hello-World/git/commits/7fd1a60b01f91b314f59955a4e4d4e80d8edf11d

{
  "sha": "7fd1a60b01f91b314f59955a4e4d4e80d8edf11d",
  "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/7fd1a60b01f91b314f59955a4e4d4e80d8edf11d",
  "html_url": "https://github.com/octocat/Hello-World/commit/7fd1a60b01f91b314f59955a4e4d4e80d8edf11d",
  "author": {
    "name": "The Octocat",
    "email": "octocat@nowhere.com",
    "date": "2012-03-06T23:06:50Z"
  },
  "committer": {
    "name": "The Octocat",
    "email": "octocat@nowhere.com",
    "date": "2012-03-06T23:06:50Z"
  },
  "tree": {
    "sha": "b4eecafa9be2f2006ce1b709d6857b07069b4608",
    "url": "https://api.github.com/repos/octocat/Hello-World/git/trees/b4eecafa9be2f2006ce1b709d6857b07069b4608"
  },
  "message": "Merge pull request #6 from Spaceghost/patch-1\n\nNew line at end of file.",
  "parents": [
    {
      "sha": "553c2077f0edc3d5dc5d17262f6aa498e69d6f8e",
      "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/553c2077f0edc3d5dc5d17262f6aa498e69d6f8e",
      "html_url": "https://github.com/octocat/Hello-World/commit/553c2077f0edc3d5dc5d17262f6aa498e69d6f8e"
    },
    {
      "sha": "762941318ee16e59dabbacb1b4049eec22f0d303",
      "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/762941318ee16e59dabbacb1b4049eec22f0d303",
      "html_url": "https://github.com/octocat/Hello-World/commit/762941318ee16e59dabbacb1b4049eec22f0d303"
    }
  ]
}

Also note that parents have "html_url" field too.

So, Commit should have HTMLURL.

They've probably added it more recently, and didn't update their old documentation examples.

amirilovic · 2017-03-30T03:26:55Z

@shurcooL Good point.

dmitshur · 2017-03-30T04:42:16Z

github/git_commits.go

@@ -22,6 +22,7 @@ type SignatureVerification struct {
 // Commit represents a GitHub commit.
 type Commit struct {
 	SHA          *string                `json:"sha,omitempty"`
+	HTMLURL      *string                `json:"html_url,omitempty"`


Move it near URL, that should be a more fitting place for it.

See

go-github/github/repos_commits.go

Line 24 in 3c8f26c

HTMLURL *string `json:"html_url,omitempty"`

.

amirilovic · 2017-03-30T19:54:55Z

@shurcooL Anything else that needs to be done here?

dmitshur

My only request is to add a blank line – see inline comment for details.

Aside from that, I don't see any issues, so this will have my LGTM after this one change request. Thanks!

dmitshur · 2017-03-30T20:05:52Z

github/search.go

+	URL         *string     `json:"url,omitempty"`
+	CommentsURL *string     `json:"comments_url,omitempty"`
+	Repository  *Repository `json:"repository,omitempty"`
+	Score       *float64    `json:"score,omitempty"`


As an observation, CommitResult now looks very similar to RepositoryCommit. But it differs in the last 2 fields. RepositoryCommit has Stats and Files, which CommitResult does not, and CommitResult has Repository and Score, that RepositoryCommit doesn't.

That's completely fine, just pointing it out.

However, I think it'd be nice to add a blank line between CommentsURL field and Repository field, to separate the common vs differing fields, similar to the blank line separator that RepositoryCommit already has.

#602)

dmitshur

LGTM. Thanks!

gmlewis

Thank you, @amirilovic and @shurcooL!

gmlewis · 2017-03-31T21:56:58Z

Merging.

There were breaking API changes in previous commit (175a6b4). Follows #602.

…oogle#602) Fix issue with commit search results deserialization Fixes google#601.

There were breaking API changes in previous commit (175a6b4). Follows google#602.

Fixing issue with commit search results deserialization #601

511dcd1

dmitshur reviewed Mar 29, 2017

View reviewed changes

Fixes #602

420bfdd

amirilovic mentioned this pull request Mar 29, 2017

Search Commits API Results Problem #601

Closed

Fixing naming issue with mapping total_count from result #602

97ffef0

gmlewis mentioned this pull request Mar 29, 2017

Consistent Go naming for JSON fields #605

Closed

amirilovic added 2 commits March 30, 2017 05:59

Reverse TotalCount commit #602

b0fc7d2

Add HTMLURL to Commit (#602)

5ab7b75

dmitshur reviewed Mar 30, 2017

View reviewed changes

Move HTMLURL near URL field (#602)

966917d

dmitshur requested changes Mar 30, 2017

View reviewed changes

Add blank line between Commit common fields and search specific fields (

3f67f5c

#602)

dmitshur approved these changes Mar 31, 2017

View reviewed changes

dmitshur requested a review from gmlewis March 31, 2017 21:45

gmlewis approved these changes Mar 31, 2017

View reviewed changes

gmlewis merged commit 175a6b4 into google:master Mar 31, 2017

dmitshur added a commit that referenced this pull request Mar 31, 2017

Bump libraryVersion to 6.

5b3f96e

There were breaking API changes in previous commit (175a6b4). Follows #602.

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Fixing issue with commit search results deserialization google#601 (g…

626d99b

…oogle#602) Fix issue with commit search results deserialization Fixes google#601.

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Bump libraryVersion to 6.

529d7fc

There were breaking API changes in previous commit (175a6b4). Follows google#602.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing issue with commit search results deserialization #601 #602

Fixing issue with commit search results deserialization #601 #602

amirilovic commented Mar 28, 2017

dmitshur Mar 29, 2017

amirilovic Mar 29, 2017

gmlewis commented Mar 29, 2017

amirilovic commented Mar 29, 2017

dmitshur commented Mar 29, 2017 •

edited

Loading

gmlewis commented Mar 29, 2017

dmitshur commented Mar 29, 2017

amirilovic commented Mar 30, 2017

dmitshur Mar 30, 2017 •

edited

Loading

amirilovic commented Mar 30, 2017

dmitshur left a comment •

edited

Loading

dmitshur Mar 30, 2017 •

edited

Loading

dmitshur left a comment

gmlewis left a comment

gmlewis commented Mar 31, 2017

Fixing issue with commit search results deserialization #601 #602

Fixing issue with commit search results deserialization #601 #602

Conversation

amirilovic commented Mar 28, 2017

dmitshur Mar 29, 2017

Choose a reason for hiding this comment

amirilovic Mar 29, 2017

Choose a reason for hiding this comment

gmlewis commented Mar 29, 2017

amirilovic commented Mar 29, 2017

dmitshur commented Mar 29, 2017 • edited Loading

gmlewis commented Mar 29, 2017

dmitshur commented Mar 29, 2017

amirilovic commented Mar 30, 2017

dmitshur Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

amirilovic commented Mar 30, 2017

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

dmitshur Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented Mar 31, 2017

dmitshur commented Mar 29, 2017 •

edited

Loading

dmitshur Mar 30, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Mar 30, 2017 •

edited

Loading