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

Adds ability to update pull request branch #1472

Merged
merged 2 commits into from
Aug 23, 2022
Merged

Adds ability to update pull request branch #1472

merged 2 commits into from
Aug 23, 2022

Conversation

bjedrocha
Copy link
Contributor

Resolves #1471

As per https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request-branch. I couldn't get the specs to work with VCR despite creating a test repo and setting all relevant ENV vars. I decided to stub the request instead.

Copy link
Contributor

@timrogers timrogers 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 taking the time to make this PR! I have a suggestion on how we could make the behaviour of the method a little nicer, and on how we test it.

# @param options [Hash] Optional parameters (e.g. expected_head_sha)
# @return [Sawyer::Resource] Info regarding the update if successful
def update_pull_request_branch(repo, number, options = {})
put "#{Repository.path repo}/pulls/#{number}/update-branch", options
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you thinking of using #boolean_from_response here? Looking at the API, it appears that the response is not super useful - you just want to know if it worked or not.

Suggested change
put "#{Repository.path repo}/pulls/#{number}/update-branch", options
boolean_from_response(:put, "#{Repository.path repo}/pulls/#{number}/update-branch", options)

@@ -136,6 +136,15 @@
end
end # new @pull methods

# stub this so we don't have to set up new fixture data
describe '.update_pull_request_branch' do
Copy link
Contributor

Choose a reason for hiding this comment

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

We should test the return value of #update_pull_request_branch in here.

@bjedrocha
Copy link
Contributor Author

Thanks for the quick review - I can address that feedback shortly. What are your thoughts on having the API called stubbed vs. having a cassette for it?

@timrogers
Copy link
Contributor

Thanks for the quick review - I can address that feedback shortly. What are your thoughts on having the API called stubbed vs. having a cassette for it?

I'm okay for it to be stubbed. Could we stub it with an actual response for better test. overage?

@bjedrocha
Copy link
Contributor Author

Addressed your feedback. I've changed update_pull_request_branch to return a simple boolean if the request succeeds. I also improved the test coverage by adding specs for the requests that fail. I didn't stub the response body as we're not doing anything with anyway but that can always be added if you think it provides value.

@tride1997

This comment was marked as spam.

@timrogers timrogers merged commit 98576f1 into octokit:main Aug 23, 2022
timrogers added a commit that referenced this pull request Aug 24, 2022
* Adds ability to update pull request branch (#1472)
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.

Update pull request branch missing?
3 participants