Add delegate powers #111
Add delegate powers #111
Conversation
50ab57c to
5211d9f
Compare
| state.add_comment(':key: Insufficient privileges') | ||
| continue | ||
| state.delegate = state.get_repo().pull_request(state.num).user | ||
| state.add_comment(':v: @{} can now approve this pull request'.format(state.delegate)) |
There was a problem hiding this comment.
Perhaps add_comment in both delegate= and delegate+?
There was a problem hiding this comment.
This was basically to mirror how r+ <commit> vs r+ works. The one where something is autoselected is where homu reports what was autoselected. I can make it report in the other one too though. shrug
5211d9f to
df2dfc4
Compare
| if try_only: | ||
| state.add_comment(':key: Insufficient privileges') | ||
| continue | ||
| state.delegate = state.get_repo().pull_request(state.num).user |
There was a problem hiding this comment.
pull_request(...).user returns a User, but it is compared to a plain string in the authentication code above (if username == state.delegate). It would be better to store pull_request(...).user.login instead here, to keep the logic simple.
df2dfc4 to
fa22c1d
Compare
|
Fixed, also added some docs in index.html |
| continue | ||
| state.delegate = state.get_repo().pull_request(state.num).user | ||
| state.add_comment(':v: @{} can now approve this pull request'.format(state.delegate)) | ||
| state.save() |
There was a problem hiding this comment.
I generally try to place relevant code together, to reduce mistakes when moving code from here to there. In this aspect, state.save() might be better placed one line above, right after state.delegate = .... Also, it would be better to insert a blank line after state.save().
There was a problem hiding this comment.
Fixed, also added the delegation message for delegate=
64844d6 to
bbea9ea
Compare
|
This is a very interesting idea. I like the feature overall, and here's a few remaining thoughts:
|
|
I'm okay with multiple delegatees. Not sure how to do that wrt the sqlite. |
|
Yeah, that was also my concern. All I can think of is to store a JSON array in the field, which is not very elegant. It would be better to land the feature first without adding multiple delegatees, then. |
146e880 to
6dcdbb2
Compare
Run `ALTER TABLE pull ADD delegate TEXT;` on the sqlite db to upgrade [breaking-change]
6dcdbb2 to
6b71d53
Compare
Not sure if homu wants this, but we're planning on using this over at Servo.
This basically lets a reviewer say "for the scope of this PR,
useris also a reviewer". Which is useful for cases where there are just minor nits left.