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-commits Fixes google#508.

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

github/search.go

+	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

github/search.go

 	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

github/search.go

@@ -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

github/search.go

+	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

github/search.go

+	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

github/search.go

 }

+// 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. type CodeResult struct {

Source:

go-github/github/search.go

Lines 115 to 116 in c9c37fd

// CodeResult represents a single search result.

type CodeResult struct {

.

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

github/search.go

+			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

github/search.go

+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

github/search.go


+// 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

github/search_test.go

+	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-commits Fixes google#508. Closes google#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 3b59f32 Jan 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.
	type CodeResult struct {

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!

dmitshur Jan 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!

dmitshur Jan 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!

dmitshur Jan 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!

dmitshur Jan 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

Uh oh!

sahildua2305 commented Jan 17, 2017 •

edited

Loading

dmitshur Jan 19, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Jan 27, 2017 •

edited

Loading

dmitshur Jan 31, 2017 •

edited

Loading

dmitshur Jan 31, 2017 •

edited

Loading