highfive icon indicating copy to clipboard operation
highfive copied to clipboard

Gracefully handle `r?` of invalid user

Open camelid opened this issue 4 years ago • 5 comments

Don't crash; instead post a comment saying that the user couldn't be assigned.

Also handle r? @ghost properly: treat it as clearing all assignees but still do the other things that highfive normally does (e.g. setting S-waiting-on-review).

r? @pietroalbini

camelid avatar Dec 06 '20 21:12 camelid

Hmm, I'm not sure how to update the tests. There are some other spots that reference the 'assignee' API field; do I need to update them too?

❯ rg "'assignee'"
highfive/newpr.py
426:                self.payload['issue', 'assignee'] \
427:                and commenter == self.payload['issue', 'assignee', 'login']

highfive/tests/test_newpr.py
911:                'assignee': None,
933:            payload._payload['issue']['assignee'] = {'login': assignee}

camelid avatar Dec 06 '20 21:12 camelid

Before this PR if an unassignable person was mentioned in a r? command the processing of the webhook completly stopped, as the exception bubbled up. This PR changes that, and the handler is called even if an unassignable person was assigned. This also means one of the features of r? @ghost (not labelling the PR and not showing "foo changed bar" comments) is now broken, as those things will still be done when people r? @ghost a PR.

The easiest fix for this is to continue raising the exception if the person is unassignable, but show the error message before if the user is @ghost. Otherwise we'd need to change all of highfive to disable things on r? @ghost.

pietroalbini avatar Dec 07 '20 10:12 pietroalbini

Skipping the S-waiting-on-review label is considered a feature of r? @ghost? I've felt that it's a bug because then wg-triage will lose PRs. I feel like people should just remove S-waiting-on-review and add S-experimental if they really want to.

not showing "foo changed bar" comments

But won't that also skip showing the submodule warning? I feel like we should treat r? @ghost PRs the same as all other PRs, the only difference being that we assign nobody.

camelid avatar Dec 07 '20 19:12 camelid

r? @ghost PRs should be treated as "null don't look" IMO, not assigned, not pinged, etc. -- minimal footprint. e.g. if I'm just opening for a perf run, I don't want to ping if I've changed submodules etc. If you do want to ping or get wg-triage to ping, do r? @Mark-Simulacrum (for example for me), i.e., assign yourself.

Mark-Simulacrum avatar Dec 07 '20 21:12 Mark-Simulacrum

Ugh, I miss you rustc!

camelid avatar Dec 07 '20 22:12 camelid