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

only accounts with merge rights should be considered as assigned reviewer #7

Open
wilzbach opened this issue Aug 18, 2016 · 12 comments

Comments

@wilzbach
Copy link
Member

Currently the bot just assigns the best (=first) reviewer without looking whether it's possible to assign him/her.

@schveiguy
Copy link

I'll note that the assignment doesn't work. Basically, it says "this PR's been assigned to you" but the assignee does not list anyone.

@wilzbach
Copy link
Member Author

Hmm this is more problematic as I thought it would be because a reviewer might be a member of the dlang organization, but still have no merge rights and thus can't be assigned.
A quick and dirty solution would probably be to loop over the list of reviewers and try to assign the next one if the GitHub PI returns an error.

@schveiguy
Copy link

An interesting thing: I had an old branch that I was waiting for another PR to be pulled before submitting. I submitted it, and got a message from dlangbot assigning to someone. Then I realized the circleci config files weren't present, so I rebased on master. At this point, it found different people to review and assign! very strange, none of my code changed, nor did the files I was touching really change much. See here: dlang/phobos#4743

@wilzbach
Copy link
Member Author

Thanks for sharing!

so I rebased on master. At this point, it found different people to review and assign! very strange, none of my code changed, nor did the files I was touching really change much

Hmm I think I can recall that @JackStouffer touched std.encoding, so it partially makes sense, but I will definitely have a closer eye on this in the future.

@dnadlinger
Copy link

A quick and dirty solution would probably be to loop over the list of reviewers and try to assign the next one if the GitHub PI returns an error.

An even quicker and dirtier solution is just to keep a whitelist with the members of "Team Phobos".

@wilzbach wilzbach changed the title only accounts wit merge rights should be considered as assigned reviewer only accounts with merge rights should be considered as assigned reviewer Aug 24, 2016
@vjeux
Copy link

vjeux commented Aug 24, 2016

One thing we found very interesting after using mention-bot on react-native is that it is now pinging a lot of people from open source that happen to have more context on some parts of the codebase than facebook employees with merge rights.

@wilzbach
Copy link
Member Author

One thing we found very interesting after using mention-bot on react-native is that it is now pinging a lot of people from open source that happen to have more context on some parts of the codebase than facebook employees with merge rights.

@vjeux Yep we observed this effect too and it's really a shame that GitHub's permission rights are so inflexible. One of the reasons why we experiment with mention-bot is that many PRs at D's standard library, runtime and compiler get stalled because no one feels responsible, hence our first idea was to automatically assign a PR to a single reviewer that has merge rights, enough background knowledge about the PR to be able to review it and is the point of contact for the submitter. So for now we plan to assign a single reviewer based on a repository whitelist, but ping multiple persons from the GitHub universe.

Btw we deployed the bot last week, so it's too early to draw a real conclusion, but I assume @andralex, @WalterBright, @yebblies, @CyberShadow @klickverbot, @burner, @schveiguy, @MartinNowak, @9il, @rainers, @JackStouffer , @aG0aep6G, or @lodo1995 are full with more improvement ideas? ;-)
FYI: @vjeux - the developer of the mention-bot - is looking for feedback, ideas and is happy to help us :)

@CyberShadow
Copy link
Member

CyberShadow commented Aug 24, 2016

Sebastian, could you please be more considerate when pinging people on GitHub. A lot of us get enough email in our inbox already. Over 90% of my GitHub email lately has been due to your @-mentions. I'd suggest avoiding @-mentions unless it's something that concerns that person directly and is blocked on their input.

@wilzbach
Copy link
Member Author

I'd suggest avoiding @-mentions unless it's something that concerns that person directly and is blocked on their input.

Fair enough, sorry for creating this noise bubble. This was stupid, I should have posted this to the NG :/
Btw if someone thinks that the bot is creating to much noise, that's a very valid opinion.

@CyberShadow
Copy link
Member

CyberShadow commented Aug 24, 2016

Btw if someone thinks that the bot is creating to much noise, that's a very valid opinion.

What has worked for me is simply blocking it. That way, I still see its comments on the web page, but don't get email. I've previously suggested that it should edit the PR description, which is what I've seen some other bots/projects do.

@CyberShadow
Copy link
Member

What has worked for me is simply blocking it.

Caveat: when a PR is assigned to me, now I no longer get a notification. We should check if editing in a @-mention triggers notifications, then perhaps we could switch to editing the original post.

@mrmonday
Copy link

mrmonday commented Aug 25, 2016

I just got mentioned as a reviewer here: dlang/dmd#6082 (comment)

As much as I appreciate the ping, its been years since I looked at dmd's source code, let alone contributed - I'd rather not be pinged in future :)

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

No branches or pull requests

6 participants