create reaction for commit comment

[shiftkey]

Link: https://developer.github.com/v3/reactions/#create-reaction-for-a-commit-comment

This is a new endpoint which should live in IRepositoryCommentsClient

POST /repos/:owner/:repo/comments/:id/reactions

Example request payload:

{
  "content": "heart"
}

As these are a limited subset of emojis, the request object should use an enumeration of values - rather than a regular string.

Example response payload:

{
  "id": 1,
  "user_id": 1,
  "content": "heart"
}

• (if not already done) define classes for payload in Octokit project and .\build FixProjects to make them available everywhere
• add new method to IReactionsClient
• add new method to IObservableReactionsClient
• implement both interfaces and ensure preview header is sent: application/vnd.github.squirrel-girl-preview
• unit tests to verify behaviour
• integration test to confirm code works against real API

[lrz-hal]

HI Shiftkey,

i'm working on this feature for commits, issues, .... Is this Ok?

[shiftkey]

@lrz-hal that's fine, thanks for the heads up!

[ryangribble]

@shiftkey I have a question over the proposed implementation of these reaction calls.

In the github API docs, these all sit underneath a top level "Reactions" section... so wouldnt that mean for octokit.net we should be creating a new ReactionsClient with the various methods under that, rather than adding CreateReaction() methods under each of the Repository PullRequest, Commit, Comment etc clients?

[shiftkey]

@ryangribble I'm not locked into the approach that I've chosen, so I can be talked off this ledge.

There's a couple of places in the API documentation where the structure doesn't reflect the underlying resources (for example, Pull Requests) and this one I'm pushing back on for a couple of reasons:

• it's new code and also a preview API
• the new endpoints build on existing resources
• for context, having these alongside existing clients might make more sense than bundling them all together

I can see from the documentation side how it's easier to group these new methods together, however it didn't feel natural to me when I was comparing it to how things are currently structured.

Thoughts?

[M-Zuber]

Documentation wise I swing either way, but for implementations - definitely feel each client that needs reactions should have such a method.

One idea that I have no idea if it's feasible as I'm not checking code is to have an interface that defines the reaction methods and then clients that expose that will provide specific implementation

[martinscholz83]

So then each client should also have a get method. Right?

[shiftkey]

@maddin2016 I've grouped up all the open issues under the reactions label:

https://github.com/octokit/octokit.net/issues?q=is%3Aissue+is%3Aopen+label%3Areactions

[ryangribble]

That's what's getting me a bit unstuck though... The delete method deletes ANY reaction, so it doesn't seem good to duplicate that on all the commit pull request issue issue comment etc clients. But having delete sitting out on it's own under a Reactions client isn't intuitive either. This is why I was questioning why we wouldn't just parallel the official documentation structure and have it all under "Reactions" client

Are there precedents where octokit doesn't align with the API docs currently? There are plenty of things we could move around and group together that would make more sense but we haven't done so (i assume because that diverges from the API documentation layout)

I'm really not phased either way I just like to clarify my own understanding of our guidelines/decisions to follow

[martinscholz83]

For me the api not feels (I dont find a better word for it) naturally. If you look at the api docs, there are root objects like gists, repositories, issues and reactions. All except reactions ( i exclude labels because i dont understand why they stay under issues. For me labels are also root objects because i can get and create labels for repositories and labels for issues. But this belongs to another discussion) implement the same schema. repos/owner/issues -> repos/owner/issues/comments repos/owner/issues/event and then comes reactions repos/owner/issues/comments/reactions. Why not repos/owner/reactions/issues repos/owner/reactions/issues/comments or repos/owner/reactions/repositories/ and so on. So for this i'm with @ryangribble. But i don't want (in germany whe say "to offend on sb. toes" 😄 ) because im very new to this. But this would maybe the way i would do it.

[shiftkey]

👍 to bringing it all under the root Reaction class. I'll go through and clarify those issues now.

[martinscholz83]

👍

[ryangribble]

fixed by #1335