Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Explain delegation more clearly #53

Closed
wants to merge 1 commit into from

Conversation

edunham
Copy link

@edunham edunham commented Jul 14, 2016

Confusion happened in servo/rust-harfbuzz#63 because Homu failed to explain what "approval" meant in the current context.

196 11:17:25 < edunham> jack: ok, how should we word homu's delegate message... "@NAME, @NAME has delegated reviewing this PR to you! Please review the code then approve it 
                        with '@bors-servo r=NAME' once you think it's ready to merge"?
196 11:18:31 < vlad> edunham: that would have made it clearer to me
196 11:18:36 < vlad> it's too bad there's no way to disable manual merges
196 11:20:30 < jdm_> edunham: delegating can also be used after the review is complete and some minor changes are still required; maybe "@DELEGATE_TARGET, @DELEGATE_SOURCE 
                     has granted you the ability to merge this PR! When the code is reviewed and ready to merge, please approve it with `@bors-servo r=NAME_OF_REVIEWER`."
196 11:21:05 < jdm_> we could conceivably use the name of the assignee instead of NAME_OF_REVIEWER
196 11:21:54 < jdm_> or we could check if the PR author == the delegate target, and suggest the name of the delegate source in that case
196 11:21:59 < jdm_> that seems like a reasonable heuristic

I'm not totally sure whether assignee is the right person to r=; is there a better value available in the state at delegation time that I should be using instead?


This change is Reviewable

Confusion happened in servo/rust-harfbuzz#63 because
Homu failed to explain what "approval" meant in the current context.
@@ -347,7 +347,9 @@ def parse_commands(body, username, repo_cfg, state, my_username, db, states, *,
state.save()

if realtime:
state.add_comment(':v: @{} can now approve this pull request'.format(state.delegate))
msg = ":v: @{}, you\'re now a reviewer for this PR!".format(state.delegate))
msg += "Once the code looks good, approve the PR by commenting '@bors-servo r=@{}'".format(state.assignee)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically don't use the @ in r=

@Manishearth
Copy link
Member

We use delegation for both telling people to r=reviewer and getting others involved in review. If PR author==delegatee, suggest r=delegator, else suggest r+

@edunham
Copy link
Author

edunham commented Mar 13, 2017

This has always been more of an issue than a viable PR so I've booted it over to #100 to clear the queue out

@edunham edunham closed this Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants