Add support for Commit Search endpoint by sahildua2305 · Pull Request #520 · google/go-github

sahildua2305 · 2017-01-17T18:51:21Z

Added support for this new preview endpoint which lets users
search for commits based on various conditions.

GitHub announcement -
https://developer.github.com/changes/2017-01-05-commit-search-api/
Docs - https://developer.github.com/v3/search/#search-commits

Fixes#508.

\cc @shurcooL @gmlewis

Added support for this new preview endpoint which lets users search for commits based on various conditions. GitHub announcement - https://developer.github.com/changes/2017-01-05-commit-search-api/ Docs - https://developer.github.com/v3/search/#search-commitsFixesgoogle#508.

dmitshur · 2017-01-17T19:32:29Z

+	AuthorName *string `json:"author_name,omitempty"`
+	AuthorEmail *string `json:"author_email,omitempty"`
+	AuthorDate *string `json:"author_date,omitempty"`
+	CommitterID int `json:"committer_id,omitempty"`


Any reason these two ints (AuthorID and CommitterID) aren't pointers, like everywhere else?

Oops! No, that's a bug, not a feature.

dmitshur · 2017-01-17T19:34:46Z

 	return result, resp, err
 }

+type SingleCommitResult struct {


golint issue here, exported struct SingleCommitResult should have documentation.

sahildua2305 · 2017-01-19T23:22:00Z

@shurcooL can you please review again? 🙂

dmitshur

I found a few opportunities to improve, and asked a few questions.

dmitshur · 2017-01-19T23:54:30Z

@@ -150,6 +182,11 @@ func (s *SearchService) search(searchType string, query string, opt *SearchOptio
 return nil, err


Update the documentation for this method. It currently says:
// ... // GitHub search types (repositories, code, issues, users)
But that's no longer true after your change, there's one more.

dmitshur · 2017-01-19T23:57:22Z

+	if searchType == "commits" {
+ // Accept header for search commits preview endpoint
+ req.Header.Set("Accept", mediaTypeCommitSearchPreview)
+	}


Combine this if statement with the if opt != nil && opt.TextMatch { below. They both decide what Accept header to set, and I think it should be exclusive (i.e., it doesn't make sense to think about the case where both run, since the second one would override the first one anyway).
A switch statement that combines their logic would work great for this. Something like:
switch { case...: req.Header.Set("Accept", ...) case...: req.Header.Set("Accept", ...) }
That'll make the code more readable and easier to maintain.

dmitshur · 2017-01-20T00:02:38Z

+	CommitterID *int `json:"committer_id,omitempty"`
+	CommitterName *string `json:"committer_name,omitempty"`
+	CommitterEmail *string `json:"committer_email,omitempty"`
+	CommitterDate *string `json:"committer_date,omitempty"`


Are you sure AuthorDate and CommitterDate should be *string instead of *Timestamp?

No, it should be *Timestamp only. Changed it.

dmitshur · 2017-01-20T00:05:22Z

 }

+// SingleCommitResult represents a commit object as returned in commit search endpoint response.
+type SingleCommitResult struct {


What do you think of following the style set forward by CodeResult and name this CommitResult?
// CodeResult represents a single search result.typeCodeResultstruct {
Source:
go-github/github/search.go
Lines 115 to 116 in c9c37fd
// CodeResult represents a single search result.
typeCodeResultstruct {
.
The prefix Single doesn't seem to be needed, is it? Unless it's needed, it's better to remove it to have a simpler struct name.

sahildua2305 · 2017-01-20T00:09:43Z

Thanks @shurcooL, I will make the changes as suggested.

- Make code consistent with the current code. - Simplified logic for setting Accept headers. - Changed AuthorDate and CommitterDate type from *string to *Timestamp.

sahildua2305 · 2017-01-22T12:26:10Z

@shurcooL I have made changes as you suggested.

sahildua2305 · 2017-01-24T23:31:46Z

@shurcooL can you please this again?

dmitshur

One change request, everything else looks good. Thanks for addressing all previous feedback!

dmitshur · 2017-01-27T00:47:06Z

+ req.Header.Set("Accept", "application/vnd.github.v3.text-match+json")
+ case searchType == "commits":
+ // Accept header for search commits preview endpoint
+ req.Header.Set("Accept", mediaTypeCommitSearchPreview)


Two things here.
Add a // TODO: remove custom Accept header when this API fully launches. comment.
Now that these cases are in the same switch, it's a little easier to reason about them. I think we should give the searchType == "commits" higher priority than opt != nil && opt.TextMatch. It may not even be valid to set opt.TextMatch: true for a commit search, but if someone does, it would fail to return any results at all because the custom accept header wouldn't be set.
So, let's be safe and put the searchType == "commits" case on top, so that the API preview header is always set when doing a commit search.

@shurcooL made the changes. Please have a look. :)

- Move commits case above the textMatch one. - Add TODO for removing custom header when API fully launches

dmitshur

LGTM. Thanks @sahildua2305!

Mind looking it over @gmlewis?

gmlewis

Thanks, @sahildua2305!
Please address the slice-of-values vs slice-of-pointers.

@shurcooL - what do you think about the field naming questions?

gmlewis · 2017-01-31T17:44:44Z

+type CommitsSearchResult struct {
+	Total *int `json:"total_count,omitempty"`
+	IncompleteResults *bool `json:"incomplete_results,omitempty"`
+	Commits []CommitResult `json:"items,omitempty"`


Please change this to:
Commits []*CommitResult`json:"items,omitempty"`
Also, wouldn't we want Items here?

Again, the current version of not using Items as the name, and not having pointers is consistent with other structs.
However, we can probably deviate on the pointers. What do you think?

I definitely think the pointer version is better despite any inconsistencies, especially for new structs. See #180 for discussion.
As for Commits, that's fine with me.

SGTM. @sahildua2305, please go ahead with this []*CommitResult change.

@gmlewis @shurcooL done 👍

gmlewis · 2017-01-31T17:48:24Z


+// CommitsSearchResult represents the result of a commits search.
+type CommitsSearchResult struct {
+	Total *int `json:"total_count,omitempty"`


Wouldn't it reduce confusion if the Go field name matched the JSON field name?
In other words, use TotalCount here?

In a normal situation, I would fully agree.
However, please look at the other similar structs in this file @gmlewis. See RepositoriesSearchResult, CodeSearchResult, etc.
They consistently call total_count as Total.

If you think it's worth changing (outside this PR), we can make a separate issue/PR to discuss that. But I suspect the cost (breaking API) wouldn't be worth the small benefit (more consistency).

gmlewis · 2017-01-31T17:50:01Z

+	want := &CommitsSearchResult{
+ Total: Int(4),
+ IncompleteResults: Bool(false),
+ Commits: []CommitResult{{Hash: String("random_hash1")}, {Hash: String("random_hash2")}},


[]*CommitResult...

gmlewis

LGTM. Thank you, @sahildua2305!

gmlewis · 2017-01-31T22:19:12Z

Merging.

gmlewis · 2017-01-31T22:23:52Z

Integrated in 3b59f32

sahildua2305 · 2017-01-31T22:30:31Z

Thanks @gmlewis

Added support for this new preview endpoint which lets users search for commits based on various conditions. GitHub announcement - https://developer.github.com/changes/2017-01-05-commit-search-api/ Docs - https://developer.github.com/v3/search/#search-commitsFixesgoogle#508. Closesgoogle#520. Change-Id: I2210fafe3da530716075f9113851e09e3bc671f8

sahildua2305 added 2 commits January 17, 2017 19:48

Fix gofmt issue with spacing between parameters
3479b35

dmitshur reviewed Jan 17, 2017
View reviewed changes

Fix: change int to *int
fda8391

dmitshur reviewed Jan 17, 2017
View reviewed changes

Fix golint issue - Add documentation for struct
f61ac61

dmitshur requested changes Jan 20, 2017
View reviewed changes

sahildua2305 added 2 commits January 22, 2017 13:04

Fix minor issues with commits search support
34fd3f0
- Make code consistent with the current code. - Simplified logic for setting Accept headers. - Changed AuthorDate and CommitterDate type from *string to *Timestamp.

Fix indentation for switch case statement
14f46c8

dmitshur requested changes Jan 27, 2017
View reviewed changes

Minor cleanup of code; minor improvements
111f86f
- Move commits case above the textMatch one. - Add TODO for removing custom header when API fully launches

dmitshur approved these changes Jan 31, 2017
View reviewed changes

dmitshur requested a review from gmlewis January 31, 2017 16:15

gmlewis requested changes Jan 31, 2017
View reviewed changes

Change slice of values to slice of pointers
ba0574c

gmlewis approved these changes Jan 31, 2017
View reviewed changes

gmlewis closed this in 3b59f32Jan 31, 2017

sahildua2305 deleted the add-commit-search branch January 31, 2017 22:30

This was referenced Mar 29, 2017
Search Commits API Results Problem #601
Closed
Fixing issue with commit search results deserialization #601 #602
Merged

gmlewis mentioned this pull request Mar 29, 2017
Consistent Go naming for JSON fields #605
Closed

		@@ -150,6 +182,11 @@ func (s SearchService) search(searchType string, query string, opt SearchOptio
		return nil, err

	// CodeResult represents a single search result.
	typeCodeResultstruct {

Uh oh!

Conversation

sahildua2305 commented Jan 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sahildua2305 commented Jan 19, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshurJan 19, 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!

sahildua2305 commented Jan 20, 2017

Uh oh!

sahildua2305 commented Jan 22, 2017

Uh oh!

sahildua2305 commented Jan 24, 2017

Uh oh!

dmitshur left a comment • 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!

dmitshurJan 27, 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!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshurJan 31, 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!

dmitshurJan 31, 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!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Jan 31, 2017

sahildua2305 commented Jan 17, 2017•
edited
Loading

dmitshurJan 19, 2017•
edited
Loading

dmitshur left a comment •
edited
Loading

dmitshurJan 27, 2017•
edited
Loading

dmitshurJan 31, 2017•
edited
Loading

dmitshurJan 31, 2017•
edited
Loading