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

[Breaking change] fix/update call to delete_reaction #1588

Merged
merged 6 commits into from
Jul 19, 2023

Conversation

dimerman
Copy link
Contributor

@dimerman dimerman commented Jun 29, 2023

BREAKING CHANGE

Couldn't find an open issue. Instead of opening a new one, I just fixed it.


Behavior

Before the change?

  • reactions created could not be deleted.

After the change?

  • reactions created could be deleted.

Other information

  • I don't think this was properly tested

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@nickfloyd nickfloyd added the Type: Bug Something isn't working as documented label Jul 18, 2023
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Hey @dimerman thank you for the contributions here! Just a few comments about the state of thing on the GitHub REST API surface. Since this is effectively a breaking change / or one where the method didn't even work I'd be inclined to even change the method name as well to more appropriately match what it does. Let me know your thoughts.

lib/octokit/client/reactions.rb Outdated Show resolved Hide resolved
lib/octokit/client/reactions.rb Outdated Show resolved Hide resolved
dimerman and others added 2 commits July 18, 2023 10:54
Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
@dimerman
Copy link
Contributor Author

👍🏻 LGTM

@dimerman dimerman requested a review from nickfloyd July 18, 2023 17:55
nickfloyd
nickfloyd previously approved these changes Jul 18, 2023
Copy link
Contributor

@nickfloyd nickfloyd 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 the contributions ❤️

@nickfloyd nickfloyd added the Type: Breaking change Used to note any change that requires a major version bump label Jul 18, 2023
@nickfloyd nickfloyd self-requested a review July 18, 2023 17:59
@nickfloyd nickfloyd dismissed their stale review July 18, 2023 18:00

needs tests to be updated

@dimerman
Copy link
Contributor Author

@nickfloyd just fixed the specs + moved the VCR cassette 👍🏻

@nickfloyd nickfloyd changed the title fix call to delete_reaction [Breaking change] fix/update call to delete_reaction Jul 19, 2023
@nickfloyd nickfloyd merged commit 54c1e6d into octokit:main Jul 19, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants