highfive
highfive copied to clipboard
Gracefully handle `r?` of invalid user
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
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}
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
.
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.
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.
Ugh, I miss you rustc
!