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.

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

Jump to bottom

Implements Topics API #1044

Merged
merged 9 commits into from
Aug 9, 2018
Merged

Implements Topics API #1044

merged 9 commits into from
Aug 9, 2018

Conversation

JuanitoFatas
Copy link
Contributor

@JuanitoFatas JuanitoFatas commented Aug 6, 2018 •
edited
Loading

This Pull Request implements Repositories REST API V3 list all topics and replace all topics endpoints.

Response for topics:

> client.topics "octokit/octokit.rb"
{:names=>["octokit", "github", "github-api"]} # A Sawyer::Resource instance

Response for replace_all_topics:

> client.replace_all_topics "juanitofatas/octokit.rb", ["octokit", "github", "github-api"]
{:names=>["github", "octokit", "github-api"]} # A Sawyer::Resource instance

Closes #978 Closes #933

@JuanitoFatas
Copy link
Contributor Author

@kytrinyx @tarebyte Please review 👀:pray:

tarebyte
tarebyte previously requested changes Aug 7, 2018
View reviewed changes
Copy link
Member

@tarebyte tarebyte left a comment

Choose a reason for hiding this comment

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

Two small nits, thanks for taking this on!

spec/octokit/client/repositories_spec.rb Outdated
@@ -273,6 +273,26 @@
end
end # .collaborators

describe ".topics", :vcr do
it "returns repository topics" do
topics = Octokit.topics("github/linguist", :accept => 'application/vnd.github.mercy-preview+json')
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using the string, could we use the value from the PREVIEW_TYPES hash?
https://github.com/octokit/octokit.rb/blob/master/lib/octokit/preview.rb#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good <3

spec/octokit/client/repositories_spec.rb Outdated
@@ -273,6 +273,26 @@
end
end # .collaborators

describe ".topics", :vcr do
Copy link
Member

Choose a reason for hiding this comment

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

Could we put these under the with repository context?

That way if we ever have to re record these cassettes another user can run them with their own data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

I am trying (by the way, there are two with repository contexts) and what test ENVs do I need in order to record cassettes correctly.

It's getting closer to my sleep time. If you could show me how to move topics spec inside one of the context will be very helpful. I can update the other one tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for not getting back to this, glad you figured it out!

jpmckinney
jpmckinney reviewed Aug 7, 2018
View reviewed changes
lib/octokit/client/repositories.rb Outdated
# Requires authenticated client for private repos.
#
# @param repo [Integer, String, Hash, Repository] A GitHub repository.
# @return [Array<Sawyer::Resource>] Hash of array representing topics.
Copy link

Choose a reason for hiding this comment

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

Do you mean "Array of hashes"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I checked the response is {:names=>["octokit", "github", "github-api"]}.

Copy link

Choose a reason for hiding this comment

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

Aha, then [Array<Sawyer::Resource>] is what needs to change.

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 already changed in f927472, thank you.

lib/octokit/client/repositories.rb Outdated
# Requires authenticated client.
#
# @param repo [Integer, String, Repository, Hash] A Github repository
# @param names [Integer] Number ID of the issue
Copy link

Choose a reason for hiding this comment

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

names is not "Number ID of the issue"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍

lib/octokit/client/repositories.rb Outdated
#
# @param repo [Integer, String, Repository, Hash] A Github repository
# @param names [Integer] Number ID of the issue
# @return [Array<Sawyer::Resource>] A list of the topics currently on the issue
Copy link

Choose a reason for hiding this comment

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

Wrong docstring for @return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's pretty embarrassing! 👍

@JuanitoFatas JuanitoFatas reopened this Aug 8, 2018
@JuanitoFatas
Copy link
Contributor Author

JuanitoFatas commented Aug 8, 2018 •
edited
Loading

@tarebyte I've moved tests into with a repository context ✨

kytrinyx
kytrinyx approved these changes Aug 8, 2018
View reviewed changes
Copy link
Contributor

@kytrinyx kytrinyx 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 great, thank you.

kytrinyx
kytrinyx previously requested changes Aug 8, 2018
View reviewed changes
Copy link
Contributor

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

Sorry, I found one thing after I approved it, that I'd like to get tidied up before we bring this in.

spec/octokit/client/repositories_spec.rb Outdated
it "replaces all topics for a repository" do
new_topics = ["octocat", "github", "github-api"]
options = {
:accept => "application/vnd.github.mercy-preview+json"
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed this the first time around. Would you please access this through the PREVIEW_TYPES constant?

Copy link
Contributor Author

@JuanitoFatas JuanitoFatas Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! Thanks!

@kytrinyx kytrinyx dismissed tarebyte’s stale review August 8, 2018 15:08

They moved the tests in.

Juanito Fatas added 9 commits August 9, 2018 09:45
@tarebyte tarebyte merged commit a779602 into octokit:master Aug 9, 2018
@tarebyte
Copy link
Member

tarebyte commented Aug 9, 2018

Thanks @JuanitoFatas!!

@JuanitoFatas JuanitoFatas deleted the jf.set-topics branch August 14, 2018 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpmckinney jpmckinney jpmckinney left review comments

@tarebyte tarebyte tarebyte approved these changes

@kytrinyx kytrinyx kytrinyx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JuanitoFatas @tarebyte @jpmckinney @kytrinyx