-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adding 4 new Reaction delete routes and obsoleting existing routes #2494
Adding 4 new Reaction delete routes and obsoleting existing routes #2494
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JonruAlveus thanks for the contributions here. I just commented on the first few files regarding some parameter naming - let me know your thoughts; the changes would need to be applied through the entire change set.
/// <param name="number">The comment id</param> | ||
/// <param name="reaction">The reaction id</param> | ||
/// <returns></returns> | ||
IObservable<Unit> Delete(string owner, string name, int number, int reaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For generation sake (future concern) and ease of consumption, I'd like to name these to match the names in the OpenAPI descriptions as closely as possible.
/// <param name="number">The comment id</param> | |
/// <param name="reaction">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(string owner, string name, int number, int reaction); | |
/// <param name="commentId">The comment id</param> | |
/// <param name="reactionId">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(string owner, string name, int commentId, int reactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/// <param name="number">The comment id</param> | ||
/// <param name="reaction">The reaction id</param> | ||
/// <returns></returns> | ||
IObservable<Unit> Delete(long repositoryId, int number, int reaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <param name="number">The comment id</param> | |
/// <param name="reaction">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(long repositoryId, int number, int reaction); | |
/// <param name="commentId">The comment id</param> | |
/// <param name="reactionId">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(long repositoryId, int commentId, int reactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/// <param name="number">The issue id</param> | ||
/// <param name="reaction">The reaction id</param> | ||
/// <returns></returns> | ||
IObservable<Unit> Delete(string owner, string name, int number, int reaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <param name="number">The issue id</param> | |
/// <param name="reaction">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(string owner, string name, int number, int reaction); | |
/// <param name="issueId">The issue id</param> | |
/// <param name="reactionId">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(string owner, string name, int issueId, int reactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/// <param name="number">The comment id</param> | ||
/// <param name="reaction">The reaction id</param> | ||
/// <returns></returns> | ||
IObservable<Unit> Delete(long repositoryId, int number, int reaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <param name="number">The comment id</param> | |
/// <param name="reaction">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(long repositoryId, int number, int reaction); | |
/// <param name="commentId">The comment id</param> | |
/// <param name="reactionId">The reaction id</param> | |
/// <returns></returns> | |
IObservable<Unit> Delete(long repositoryId, int commentId, int reactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@nickfloyd all changes made! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes here! I was looking at Octokit/Helpers/ApiUrls.cs, and it looks like that whole class needs to be refactored to fit a less obscure naming scheme - lots of use of the nondescript number
and the like.
I've created an issue for that one so that we can move forward with your changes here.
release_notes: Adds 4 new Reaction delete routes to replace the existing obsolete routes |
fixes #2490
#2430