add reactions for commit comments fixed #1302 #1325
Conversation
|
|
||
| client.CreateReaction("fake", "repo", 1, newCommitCommentReaction); | ||
|
|
||
| connection.Received().Post<CommitCommentReaction>(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/comments/1/reactions"), Arg.Any<object>(), AcceptHeaders.Reactions); |
There was a problem hiding this comment.
You should hardcode the header string, so it serves as a sanity check against misspelling.
|
Overall looks pretty solid to me. |
|
Jap, @lrz-hal is my "now" my testaccount. I have run the integration tests 😄 |
|
When I first started contributing to octokit.net I did the same thing 😆 |
|
🎉 |
|
Thanks!! If this looks good i will continue on reactions for other types like issues, ... |
|
Hi @maddin2016 many thanks for your first contribution! In general everything looks pretty good. I do have an outstanding query with @shiftkey on the corresponding issue #1302, regarding his proposed design/location of these reaction calls as it seemed like putting them under each section (comment, commit, pull request, issue section, etc) is not inline with the github API docs/structure which I thought was a general goal/rule in octokit,net to stay as close to the official API as possible. But in the meantime while we wait for an answer, we can work through the changes you've submitted and I'll highlight a few minor consistency/nitpick type things to get you familiar with contributing to the project 👍 Also as an FYI, when working on PR if you mention the magic words "fixes #1302 in your PR body, then when we merge the PR it will automatically mark the issue as closed... so it's a good habit to get into 😀 |
| @@ -367,4 +367,62 @@ public void Dispose() | |||
| _context.Dispose(); | |||
| } | |||
| } | |||
| public class TheReactionMethod : IDisposable | |||
There was a problem hiding this comment.
should insert one blank line in between class declarations
There was a problem hiding this comment.
the name of the test class should reflect the name of the method. In this case the method is called CreateReaction so the test class should be called TheCreateReactionMethod
|
Hi @ryangribble. Many thanks. I'm very excited to make my first pull request for a project 😃 In the meantime i have created a new standalone ReactionClient where i implement a Delete Method like in #1303. Should i create a new branch and make a pull request. Or should i wait until this pull request is closed or adopted? |
| } | ||
|
|
||
| [DebuggerDisplay("{DebuggerDisplay,nq}")] | ||
| public class CommitCommentReaction |
There was a problem hiding this comment.
This response object is returned for creating reactions on CommitComments, Issues, IssueComments and PullRequestReviewComments, so it should probably be called something more generic like Reaction rather than CommitCommentReaction
| { | ||
| public enum Reaction | ||
| { | ||
| [Parameter(Value = "plus_one")] |
There was a problem hiding this comment.
The api doco seems to indicate the string is "+1" and "-1" for these, but you've got their API string value as plus_one and minus_one. Just checking if this was intentional?
|
Feel free to discuss any of the feedback if you don't agree! I try to give detailed feedback around things, simply to get contributors into the groove of our approach/thinking. Sometimes this may come across as critical but that's not the intention! We appreciate all contributions and are very keen to work with you to ensure you have good experiences contributing to open source dotnet projects 😀 |
|
Ok. I will do this in the future. Thanks for all the suggestions |
| var newCommitCommentReaction = new NewCommitCommentReaction(Reaction.Confused); | ||
| var reaction = await _github.Repository.Comment.CreateReaction(_context.RepositoryOwner, _context.RepositoryName, result.Id, newCommitCommentReaction); | ||
|
|
||
| Assert.IsType<CommitCommentReaction>(reaction); |
There was a problem hiding this comment.
Maybe a good idea to assert the fields of the response message (eg confirm that the ReactionType you requested is what was actually set). Also given the other comment about plus_one vs +1 perhaps looping through all of the available ReactionType enum values and creating each one woudl be good, just to prove that all of them are defined correctly
|
PS, with the "fixed #1302" tip i mentioned, even if you add that now (by editing your PR body) it will close the issue when the PR is merged. |
martinscholz83
changed the title
|
Another thing i would like to discuss. So far we have a request client |
|
@maddin2016 yeah, I don't see the point in duplicating all the types here where they're structurally the same... |
|
Any ideas for naming |
|
ReactionType was my suggestion. That matches the terminology in API docs too... Also the more we go through these specifics (request response types are common across all, there's only a single delete method for any reaction etc), the more I reckon it makes sense it all lives under a top level "Reactions" client. If you're happy to do it, I'd love to see a PR providing that option so we can see how it feels |
|
Ok. Then i will start on this. I leave a comment for this topic #1302 |
|
Is there any experimental branch where i can create pull requests? |
|
Well all PR's are experimental until they are merged or abandoned so feel free to raise a PR for whatever you want |
|
Closing this out in favour of #1335 which is going for the standalone client approach. |
Added feature to create reactions for commit comments.
To Do
Tests fails because of master branch!