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

Allow additional information on the comment that approves/denies #21

Open
Phoenix616 opened this issue Jun 21, 2022 · 7 comments
Open
Labels
enhancement New feature or request

Comments

@Phoenix616
Copy link

Phoenix616 commented Jun 21, 2022

Currently the messages have to stand in their own comment. It would be ideal to be able to to provide additional information on new lines after the keyword especially in the case of denials to provide some kind of reason in an easy way (instead of having to post two comments)

Also while I'm at that: #12 did not fix #8 for denials.

@trstringer
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

Phoenix616 commented Jun 22, 2022

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
Copy link
Owner

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

@trstringer trstringer added the enhancement New feature or request label Jun 23, 2022
@alex-ld
Copy link

alex-ld commented May 4, 2023

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants