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

Rename PulRequest.Comment client to PullRequest.ReviewComment #1511

Closed
ryangribble opened this issue Dec 12, 2016 · 4 comments
Closed

Rename PulRequest.Comment client to PullRequest.ReviewComment #1511

ryangribble opened this issue Dec 12, 2016 · 4 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone

Comments

@ryangribble
Copy link
Contributor

To better align with the API docs, the access to the PullRequestReviewClient should be renamed from client.PullRequest.Comment to client.PullRequest.ReviewComment (also applies to the Observable versions of the client)

Normal octokit.net deprecation procedures should be followed (old variables being marked [Obsolete] for at least 2 releases before being deleted).

@bmeverett
Copy link
Contributor

Is this what you're looking for? And then should the tests be modified to use ReviewComment now?

@ryangribble
Copy link
Contributor Author

Yeah pretty much, although the obsolete message wording should be more in line with other examples in the code base. It also needs to be done in both the interface definition and the concrete implementation and also for the Observable client/interface

And yes updating any other test code should be done now, so that later on the removal of the deprecated endpoint is simple/surgical

@bmeverett
Copy link
Contributor

Ok and just to be clear about the Observables. That would require changing IObservablePullRequestReviewCommentsClient.GetComment(long repositoryId, int number) and IObservable<PullRequestReviewComment> GetComment(string owner, string name, int number) correct?

And does anything need to be done with Task<PullRequestReviewComment> GetComment(string owner, string name, int number)?

@ryangribble
Copy link
Contributor Author

What I actually meant was to make the same rename to the variable name in the observable API client structure... But you are spot on that the internals of the observable methods will also need to be tweaked to call the new client's path

If you're keen to work on this just push up a PR with your progress and we can work through it there 👍

@nickfloyd nickfloyd added Status: Up for grabs Issues that are ready to be worked on by anyone and removed up-for-grabs labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone
Projects
None yet
Development

No branches or pull requests

3 participants