Skip to content
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.

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

Add support for the Commit Search API preview #959

Merged
merged 9 commits into from
Mar 6, 2018

Conversation

azizshamim
Copy link
Contributor

This PR adds support for the Commit Search API preview:

  • Adds a new search_commits method for returning the search results for commits
  • Adds documentation for the new method (it's basically the same as the other search methods)

@azizshamim
Copy link
Contributor Author

I know I need a new cassette. The code works in script/console. Not entirely sure why a new cassette isn't being generated when I run the tests.

@coveralls
Copy link

coveralls commented Nov 2, 2017

Coverage Status

Changes Unknown when pulling 25a9433 on azizshamim:commit-search-preview into ** on octokit:master**.

@azizshamim
Copy link
Contributor Author

azizshamim commented Dec 7, 2017

@tarebyte - any tips on getting the right fixtures into this branch? I'm struggling to generate them.

@tarebyte
Copy link
Member

tarebyte commented Dec 7, 2017

@azizshamim everything seems to be ok, I'll take a look to see if something funky is going on.

@azizshamim
Copy link
Contributor Author

@tarebyte I'm about 90% sure it's because I don't have the right credentials set up, nor the right configuration set up, to connect to GitHub API and generate the right fixture for the tests. Any guidance would be much appreciated.

Copy link

@cheshire137 cheshire137 left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me!

#
# @param query [String] Search terms and qualifiers
# @param options [Hash] Sort and pagination options
# @option options [String] :sort Sort field

Choose a reason for hiding this comment

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

Maybe Field to sort by instead of Sort field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was matching the language of the other option descriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the rest of the codebase uses Sort field. Some endpoints also mention what the available sort options are, but many do not.

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 5, 2018

@azizshamim Did you define OCTOKIT_TEST_GITHUB_TOKEN with a valid personal token when you ran the tests?

@azizshamim
Copy link
Contributor Author

@kytrinyx - Yes. Just retested it, same error.

  1) Octokit::Client::Search.search_commits searches commits
     Failure/Error: results = @client.search_commits 'repo:octokit/octokit.rb author:jasonrudolph', \
     NoMethodError:
       undefined method `<<' for {:read_timeout=>60, :continue_timeout=>nil, :debug_output=>nil}:Hash
       Did you mean?  <

Does the token need any specific permissions?

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 6, 2018

Does the token need any specific permissions?

No, it shouldn't. Let me pull this down and see if I can figure out what's going on.

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 6, 2018

This worked for me with a token that didn't have any scopes. I've pushed up the VCR cassette. If this passes I'll go ahead and merge it.

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 6, 2018

Thanks for your patience in getting this change in! ✨

@kytrinyx kytrinyx merged commit 86362b9 into octokit:master Mar 6, 2018
@azizshamim
Copy link
Contributor Author

Thanks for the assist @kytrinyx. I wonder if I’ve borked my global vcr setup somehow.

I’d like to add support for all the API previews that aren’t implemented to this gem. I wonder if I can fix it before I open the next Pull Request.

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 6, 2018 via email

@azizshamim
Copy link
Contributor Author

did you delete the generated cassette between each attempt?

The cassette never regenerates if I delete it. None of them do. Whether I have a valid token in OCTOKIT_TEST_GITHUB_TOKEN or not.

@kytrinyx
Copy link
Contributor

kytrinyx commented Mar 8, 2018

Ok, that totally sucks! I have no idea what might be going on here.

@tarebyte
Copy link
Member

tarebyte commented May 8, 2018

This has been released as part of https://github.com/octokit/octokit.rb/releases/tag/v4.9.0

@azizshamim azizshamim deleted the commit-search-preview branch May 8, 2018 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants