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 remove_assignees method to the client #984

Merged
merged 2 commits into from
Feb 23, 2018
Merged

Add remove_assignees method to the client #984

merged 2 commits into from
Feb 23, 2018

Conversation

licatajustin
Copy link
Contributor

@licatajustin licatajustin commented Jan 26, 2018

Issue
Closes #983

Context
GitHub exposes an API to remove assignees from an Issue/Pull Request. This Pull Request takes advantage of that by adding a public method exposed in Octokit::Client.

Please refer to GitHub's API for reference.

Usage:

Octokit.remove_assignees("octokit/octokit.rb", 23, ["pengwynn", "joeyw"])

@licatajustin
Copy link
Contributor Author

@tarebyte, anything I can do to help get this merged?

@tarebyte tarebyte self-assigned this Jan 31, 2018
@tarebyte tarebyte self-requested a review January 31, 2018 22:42
@tarebyte tarebyte removed their assignment Jan 31, 2018
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.

Thanks for tackling this @licatajustin

I have one bit of feedback and then this should be good to 🚢

# @see https://developer.github.com/v3/issues/assignees/#remove-assignees-from-an-issue
# @example Remove assignees "pengwynn" and "joeyw" from Issue #23 on octokit/octokit.rb
# Octokit.remove_assignees("octokit/octokit.rb", 23, ["pengwynn", "joeyw"])
def remove_assignees(repo, number, assignees)
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the normal patter of an options hash as the last argument, would you mind adding it?

def remove_assignees(repo, number, assignees, options = {})
  # ...
end

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. Just updated the commit.

@licatajustin
Copy link
Contributor Author

Let me know if anything else is needed @tarebyte!

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.

Would you also update the documentation? Then this should be good to 🚢

# @param repo [Integer, String, Repository, Hash] A GitHub repository
# @param number [Integer] Issue number
# @param assignees [Array] Assignees to be removed
# @return [Sawyer::Resource] Issue
Copy link
Contributor

Choose a reason for hiding this comment

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

We're missing the options hash in the documentation here.

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 @kytrinyx for the review.

What are you expecting here? The options argument doesn't seem very useful outside of consistency efforts with the rest of this library, as the GitHub API doesn't allow for additional parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing something, the options hash is where you would pass things like custom accept headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate it! 🌷

Copy link
Contributor

Choose a reason for hiding this comment

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

Dang. I was a bit quick to merge. Still not seeing the options hash—I'll add it in a separate PR.

licatajustin and others added 2 commits February 23, 2018 13:50
GitHub exposes an API to remove assignees from an Issue/Pull Request.
This Pull Request exposes an API for the octokit library to take
advantage of this endpoint.

Usage:

  Octokit.remove_assignees("octokit/octokit.rb", 23, ["pengwynn", "joeyw"])
@kytrinyx kytrinyx dismissed tarebyte’s stale review February 23, 2018 22:04

The issues were addressed.

@kytrinyx kytrinyx merged commit 21d919d into octokit:master Feb 23, 2018
@licatajustin licatajustin deleted the feature/remove-assignees-feature branch February 23, 2018 22:17
@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

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.

3 participants