Skip to content

Add support for Commit Search endpoint #520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Add support for Commit Search endpoint #520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
730d49f
Add support for Commit Search endpoint
sahildua2305 Jan 17, 2017
3479b35
Fix gofmt issue with spacing between parameters
sahildua2305 Jan 17, 2017
fda8391
Fix: change int to *int
sahildua2305 Jan 17, 2017
f61ac61
Fix golint issue - Add documentation for struct
sahildua2305 Jan 17, 2017
34fd3f0
Fix minor issues with commits search support
sahildua2305 Jan 22, 2017
14f46c8
Fix indentation for switch case statement
sahildua2305 Jan 22, 2017
111f86f
Minor cleanup of code; minor improvements
sahildua2305 Jan 31, 2017
ba0574c
Change slice of values to slice of pointers
sahildua2305 Jan 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions github/search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,27 +55,28 @@ func (s *SearchService) Repositories(query string, opt *SearchOptions) (*Reposit
return result, resp, err
}

// SingleCommitResult represents a commit object as returned in commit search endpoint response.
type SingleCommitResult struct {
// CommitsSearchResult represents the result of a commits search.
type CommitsSearchResult struct {
Total *int `json:"total_count,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dmitshur dmitshur Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, SGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

IncompleteResults *bool `json:"incomplete_results,omitempty"`
Commits []CommitResult `json:"items,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to:

Commits           []*CommitResult `json:"items,omitempty"`

Also, wouldn't we want Items here?

Copy link
Member

@dmitshur dmitshur Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlewis @shurcooL done 👍

}

// CommitResult represents a commit object as returned in commit search endpoint response.
type CommitResult struct {
Hash *string `json:"hash,omitempty"`
Message *string `json:"message,omitempty"`
AuthorID *int `json:"author_id,omitempty"`
AuthorName *string `json:"author_name,omitempty"`
AuthorEmail *string `json:"author_email,omitempty"`
AuthorDate *string `json:"author_date,omitempty"`
AuthorDate *Timestamp `json:"author_date,omitempty"`
CommitterID *int `json:"committer_id,omitempty"`
CommitterName *string `json:"committer_name,omitempty"`
CommitterEmail *string `json:"committer_email,omitempty"`
CommitterDate *string `json:"committer_date,omitempty"`
CommitterDate *Timestamp `json:"committer_date,omitempty"`
Repository *Repository `json:"repository,omitempty"`
}

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

// Commits searches commits via various criteria.
//
Expand Down Expand Up @@ -168,7 +169,7 @@ func (s *SearchService) Code(query string, opt *SearchOptions) (*CodeSearchResul
}

// Helper function that executes search queries against different
// GitHub search types (repositories, code, issues, users)
// GitHub search types (repositories, commits, code, issues, users)
func (s *SearchService) search(searchType string, query string, opt *SearchOptions, result interface{}) (*Response, error) {
params, err := qs.Values(opt)
if err != nil {
Expand All @@ -182,15 +183,14 @@ func (s *SearchService) search(searchType string, query string, opt *SearchOptio
return nil, err
Copy link
Member

@dmitshur dmitshur Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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

if opt != nil && opt.TextMatch {
// Accept header defaults to "application/vnd.github.v3+json"
// We change it here to fetch back text-match metadata
req.Header.Set("Accept", "application/vnd.github.v3.text-match+json")
switch {
case opt != nil && opt.TextMatch:
// Accept header defaults to "application/vnd.github.v3+json"
// We change it here to fetch back text-match metadata
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)
Copy link
Member

@dmitshur dmitshur Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here.

  1. Add a // TODO: remove custom Accept header when this API fully launches. comment.

  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return s.client.Do(req, result)
Expand Down
2 changes: 1 addition & 1 deletion github/search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestSearchService_Commits(t *testing.T) {
want := &CommitsSearchResult{
Total: Int(4),
IncompleteResults: Bool(false),
Commits: []SingleCommitResult{{Hash: String("random_hash1")}, {Hash: String("random_hash2")}},
Commits: []CommitResult{{Hash: String("random_hash1")}, {Hash: String("random_hash2")}},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[]*CommitResult...

}
if !reflect.DeepEqual(result, want) {
t.Errorf("Search.Commits returned %+v, want %+v", result, want)
Expand Down