Allow additional information on the comment that approves/denies about manual-approval

[trstringer]

Thanks for opening up this issue. And wow good catch for #8 for denials! I'll fix that asap.

Regarding the original issue, I'm slightly inclined to continue the workflow of having multiple comments for additional context.

So you feel as though it would be better if this was a single comment?

denied.

Here is the reason that it was denied.

[Phoenix616]

So you feel as though it would be better if this was a single comment?

denied.

Here is the reason that it was denied.

Yes. The reasoning behind this would be that with the current implementation one would have to pre-write the approval/denial reason somewhere and then copy it into another comment after posting the approval/denial comment if one would want the reasoning to be available instantly. Which imo. can be important as people might be pretty fast in checking especially if something was denied so you would want the reason to be there too as soon as possible.

[trstringer]

Good point. Maybe I should convert this into a slash command. So it would match on /deny, /lgtm, etc. What do you think? Would be easier to see "hey that's a command that will be handled" as opposed to just a normal string like current.

[Phoenix616]

Hm, I kinda like the distinction but if anything I think that should be optional or configurable. (Or just apply to comments that have additional text) Otherwise I could imagine that such a change could break quite some people's workflows/lead to delays until the change actually reached everyone who's working with it.

[trstringer]

Yeah I definitely can't break existing v1 users. I'd have to put the slash command behind a feature flag.

[alex-ld]

Reading this thread makes me think what if keywords for both approval and denial occur at the same time in the comment? Which one will take precedence?

[Phoenix616]

Reading this thread makes me think what if keywords for both approval and denial occur at the same time in the comment? Which one will take precedence?

Shouldn't be an issue imo if only the first line is checked like in my original suggestion. Otherwise I would expect to eifher the first occurence to count or an error beign returned about ambigiousness.