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.

Sign up for GitHub

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

Jump to bottom

[Proof of concept] Show hello greeting message #44

Merged
merged 1 commit into from
Jun 22, 2017
Merged

[Proof of concept] Show hello greeting message #44

merged 1 commit into from
Jun 22, 2017

Conversation

wilzbach
Copy link
Member

@WebDrake - I hope you still have time & motivation:

Assuming that the bot can be relied on to be 'first responder' to any PR, I'm happy to try to draft an alternative text for it to post (and maybe also look at what texts it can link to).

Here's a proof of concept change that would make the dlang-bot show such a "hello contributor" message.
I didn't adapt the test suite as this PR is just to show that's rather trivial to add such a message ;-)

@MartinNowak what's your opinion on this? If there are people who consider this as too noisy, they could configure a filter at their mail server or simply block the dlang-bot user. OTOH it might be quite motivating and helpful to have a handy summary of important links. Moreover, the dlang-bot is already comments on about half of all dlang PRs.

For reference - the starting post

I have a PR of my own that's been in this situation for (only!) a month now, and it's distinctly frustrating, particularly because it was a contribution that Andrei specifically called for on these forums
Contrast this with the experience I had the one time I submitted a (tiny, trivial) patch to rust: immediately after submitting the PR I got a message from their 'highfive' robot that included:

  • a friendly thank you for the PR;
  • the GitHub ID of a contact who I could expect to be taking responsibility
    for the PR, who was also assigned as a reviewer;
  • some helpful notes on how to add changes to the PR if requested;
  • a link to the contributor guidelines.
    By contrast with a Phobos PR it's not clear who to contact if review or decision-making is not forthcoming.
    There's clearly in part a scaling problem here (in terms of how many people are available in general, and in terms of how many people have expertise on particular parts of the library) but it also feels like a few simple things (like making sure every PR author is given a reliable contact or two who they can feel entitled to chase up) could make a big difference.

@WebDrake
Copy link

Hi @wilzbach -- of course I have time and motivation for stuff like this, don't be misled if I seem grumpy on the forums ;-)

The message I would like to see probably goes something like this:

Thanks for your pull request, {contributor}!  We are looking
forward to reviewing it, and you should be hearing from
{reviewer-with-commit-rights} (or another of our maintainers)
soon.

Some things that can help to speed things up:

* smaller, focused PRs are easier to review than big ones

* try not to mix up refactoring or style changes with bug
  fixes or feature enhancements

* provide helpful commit messages explaining the rationale
  behind each change

Bear in mind that large or tricky changes may require multiple
rounds of review and revision.

Please see {link} for more information.

where the word maintainers should be a link to a page of people with commit rights to the repo in question. We could possibly use GitHub teams for this purpose (not necessarily to limit commit rights but in order to create more focused lists of maintainers for the different repos).

I've left out the auto-suggestion of reviewers because I think this does risk being spammy. It's better for that information to be available (on request) to the reviewer with commit rights, who can then decide who to ping and how to do it.

The soon part of this is of course a tricky bit, because it relies on people being willing to be responsive ... ;-)

What do you think?

BTW I would be careful to ensure this kind of message is cleanly separated from things like the bot's cross-linking to issues.dlang.org.

@MartinNowak
Copy link
Member

First thoughts:

Isn't this what CONTRIBUTING.md is supposed to do?
Would indeed be fairly noisy for the 10th PR of someone, might make more sense for first time contributors (which still require additional whitelisting from our side atm.)
None of those things help to get more resources for reviews or lower the mental cost of context switching/multi-tasking.
What I would find helpful from a reviewer perspective is a customized/filterable list of PRs that I'd be well-suited to review, and maybe another list with trivial PRs.
Looking at any of our PR lists just has a paralysing effect on me, not to mention the almost random GH notifications.

@WebDrake
Copy link

Isn't this what CONTRIBUTING.md is supposed to do?

The {link} in the last sentence should be to CONTRIBUTING.md. The idea of the message text is that it's a little nudge on the really essential stuff, so that people can think, "Oh shit, I didn't do that, let me fix that now" while the PR is still hot in their minds (and maybe re-read the full contributing guidelines if they forgot to do so).

We could probably reduce the number of points, though.

Would indeed be fairly noisy for the 10th PR of someone, might make more sense for first time contributors (which still require additional whitelisting from our side atm.)

Yes, good point. Could we send a different message for people who GitHub already considers a contributor? Maybe just a shorter message assigning a reviewer with commit rights?

None of those things help to get more resources for reviews or lower the mental cost of context switching/multi-tasking.

No, I agree. But I do think there's some value in setting a tone of expectation in terms of responsiveness of someone responsible for the project. Psychological 'nudges' can be for maintainers as well.

It would need 'buy-in' though from people with commit rights. I wouldn't feel comfortable with this message being approved if maintainers felt it was making false promises on their behalf.

What I would find helpful from a reviewer perspective is a customized/filterable list of PRs that I'd be well-suited to review, and maybe another list with trivial PRs.
Looking at any of our PR lists just has a paralysing effect on me, not to mention the almost random GH notifications.

Good point that could maybe be handled separately -- perhaps maintainers could receive a weekly email identifying 5 PRs for which their attention could be useful?

By the way, for reference, the 'highfive' bot message that I received when submitting to rust was as follows. I did rather follow its style but tweaked some things for D:

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from {maintainer} (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

What is interesting about this of course is that it only mentions points regarding the upcoming review, not about how the PR should be prepared.

@WebDrake
Copy link

Might be worth covering what we want to get out of this.

Fundamentally, my own goals for this kind of auto-response would be:

  • every PR gets a maintainer assigned to it who the PR author can feel free to contact and/or push

  • in addition to their assigned maintainer, the PR author gets provided with a link to additional contacts who they can follow up with if their PR doesn't get a timely response

  • the PR author gets a minor nudge about good practice in submitting PRs and revisions to their PR

  • the maintainer has a clear list of PRs for which they are responsible (with the opportunity to exchange responsibilities with other maintainers if they are too busy/don't feel qualified to steward the review)

@MartinNowak, does that sound reasonable to you? Is there anything you would add/remove or which you see as problematic?

@MartinNowak
Copy link
Member

Might be worth covering what we want to get out of this.

Fundamentally, my own goals for this kind of auto-response would be:

every PR gets a maintainer assigned to it who the PR author can feel free to contact and/or push

in addition to their assigned maintainer, the PR author gets provided with a link to additional contacts who they can follow up with if their PR doesn't get a timely response

the PR author gets a minor nudge about good practice in submitting PRs and revisions to their PR

the maintainer has a clear list of PRs for which they are responsible (with the opportunity to exchange responsibilities with other maintainers if they are too busy/don't feel qualified to steward the review)

@MartinNowak, does that sound reasonable to you? Is there anything you would add/remove or which you see as problematic?

I'm not positive that any push-based assignment will work out, we're fairly busy already and more pushing will rather lead to even more congestion and unfocused hopping around.
We could streamline pull-based PR reviews, i.e. customized list of reviewable PRs, to make better use of reviewer time.
In any case, better responses require a more organized approach from maintainers, trying to enforce this bottom-up will just be frustrating.

Identifying (or mentioning) common PR mistakes/practices is indeed interesting (also see #8) to reduce the amount of review ping-pong. Can we get some proper stats/analysis on why PRs get stuck or take too long to be merged?

@WebDrake
Copy link

WebDrake commented Feb 16, 2017 •
edited
Loading

I'm not positive that any push-based assignment will work out, we're fairly busy already and more pushing will rather lead to even more congestion and unfocused hopping around.

Fair point. I'm less concerned about pushing maintainers to do things, than about what can be done to make sure that no PR misses out on a timely maintainer response.

It's obviously better if volunteer maintainers are able to self-select what they work on, but there must be plenty of things that can be done (in a non-pushy way) to facilitate awareness of which PRs need particular kinds of action.

In any case, better responses require a more organized approach from maintainers, trying to enforce this bottom-up will just be frustrating.

I wonder if we might be able to get some traction by reliably identifying statuses of a PR:

  • needs review [there has been no 'official' review provided by a maintainer at all, in the sense of GitHub's review feature];

  • needs extra review [assuming that a maintainer has reviewed a PR, a second or even third opinion might still be needed -- this should probably be requested by the last reviewer rather than expected by default];

  • needs decision [when enough favourable reviews are available];

  • changes or feedback needed [when author action is required to proceed];

  • auto-merge [when a decision to accept the PR is made].

In the event of changes to the submitted patches, the status would reset to 'needs review'.

Of the above I think that only 'needs review' and 'auto-merge' are available among the current tags.

Can we get some proper stats/analysis on why PRs get stuck or take too long to be merged?

No promises that it'll be soon, but I'll try to do a quick examination of open phobos PRs and see if I can classify them in some way (possibly along the lines of the above statuses).

@wilzbach
Copy link
Member Author

Good point that could maybe be handled separately -- perhaps maintainers could receive a weekly email identifying 5 PRs for which their attention could be useful?

See also #46 - during the summer I did this manually for a couple of weeks by sending @andralex a selected list of PRs that required his attention. It worked quite well, we went down to 60 PRs at Phobos for the time being. Unfortunately it's quite a lot of work, but we could do something like @WebDrake proposed:

  • dlang-bot assigns the "needs-review" label by default
  • the weekly list of PR gets randomly selected out of the pool of PRs that are in stage 1 - "needs-review" (or stage1b "needs-second-review")
  • once there's a formal review, of course the "needs-review" labels should be removed automatically

Maybe this could even be done with solely with new GitHub Review feature

I'm not positive that any push-based assignment will work out, we're fairly busy already and more pushing will rather lead to even more congestion and unfocused hopping around.

FWIW there's a designated issue about automatically assigning PRs: #31

I wonder if we might be able to get some traction by reliably identifying statuses of a PR:

I would add sth. like "no-activity" to the list as handling stalled PRs is especially difficult.
See also: #47 (auto-warn/close old PRs with no activity)

My 2 cents

  • We already post the first comment on roughly half of the PRs, so it won't add too much extra noise and people who dislike the Dlang-Bot notifications have it already blocked (they can simply block the bot or setup email filters if they want to be more specific)
  • It's quite useful if we want to display additional help like:
  • It doesn't "hurt" anyone, but provides guidance and motivation to newcomers

@WebDrake
Copy link

BTW, just a slightly extended thought on how the above 'PR status' ideas could be helped by dlang-bot.

Let's suppose a standard model for approval of a PR which goes along the lines of, there need to be two favourable maintainer reviews (and no outstanding changes requested) before a change can be accepted. However, maintainers can override this in the event of trivial PRs which don't need that much scrutiny.

So, the possible interactions would go something like:

  • PR arrives. dlang-bot greets the author. Assuming sufficient CI tests pass, it auto-labels the PR: 'needs review'.

    • 'sufficient CI tests pass' would normally mean all CI tests pass, but that condition could be relaxed if some CI tests are deemed to be advisory only or if their status is known to be flaky

    • the author greeting need not auto-nominate any reviewers (hence not spammy for maintainers)

  • After the first maintainer review arrives (either approval or changes requested, comments don't count), dlang-bot updates the label to either 'needs extra review' or 'author action needed'

    • assuming a positive review, the maintainer is at liberty to update the label manually to 'needs decision' or even 'auto-merge' if they see fit

  • if the current label is 'needs extra review' then another maintainer approval will cause dlang-bot to update the label to 'needs decision'

    • even assuming a positive review, maintainers are at liberty to reset the label manually to 'needs extra review' if they want a 3rd or 4th or nth opinion

  • possibly, if the current label is 'needs decision', dlang-bot might ping the PR reviewers if more than 3 weeks go by with no maintainer action

  • manual action is always required to set the label to 'auto-merge'

  • if the PR author updates the patches, dlang-bot will reset the tag to 'needs review'

  • if the current label is 'author action needed', dlang-bot will ping the author after 2 weeks of inactivity (== 2 weeks since the author last did anything to the PR, including comments)

    • after 4 weeks of inactivity it will issue a notice that the PR will be auto-closed in 2 weeks unless the author responds

    • after 6 weeks of inactivity, dlang-bot will auto-close the PR with an apologetic message inviting the author to re-open or re-submit the PR if they want to continue with their submission.

Does that sound like a helpful structure (without too much push on the maintainers)?

@wilzbach
Copy link
Member Author

Does that sound like a helpful structure (without too much push on the maintainers)?

Yep it does to me! I added a couple of comment for the individual points below.

'sufficient CI tests pass' would normally mean all CI tests pass, but that condition could be relaxed if some CI tests are deemed to be advisory only or if their status is known to be flaky

GitHub allows to have "required" CI tests (at Phobos: auto-tester, DAutoTest) and non-required for now are (CircleCi, CodeCov and the ProjectTester).
What I would find very useful if the dlang-bot would tag PRs that have errors with "has-error", "needs-work" or "author action needed" (and of course automatically untag them). The idea behind this would be also notify the author when his PR is in an mergeable state or has persistent build error. I wasn't sure whether you implied this.

See also: #43 (trigger warning when a PR gets unmergeable)

if the current label is 'author action needed', dlang-bot will ping the author after 2 weeks of inactivity (== 2 weeks since the author last did anything to the PR, including comments)

I think the time intervals need to be larger, but they definitely should exist.

possibly, if the current label is 'needs decision', dlang-bot might ping the PR reviewers if more than 3 weeks go by with no maintainer action

I think that's a good idea. It could here draw reviewers randomly from an OWNERS file or the history of the git diff (the later has the disadvantage that often non-core contributors are selected).

@WebDrake
Copy link

I think that's a good idea. It could here draw reviewers randomly from an OWNERS file or the history of the git diff (the later has the disadvantage that often non-core contributors are selected).

In this case I wasn't thinking of pinging arbitrary reviewers, but those maintainers who had already submitted a GitHub review (& so are already self-engaged with the PR).

@wilzbach
Copy link
Member Author

@WebDrake I went ahead and started to implement a first iteration (or grounding layer) for the features you proposed.

Once we have all these building bits in place, we can stark looking on the details of the 'needs review' cycle. From what I understand it's supposed to be and in a simplified way:

  1. on opened: PR greeting

  2. on synchronize, remove any of these labels: "needs work", "needs rebase" (Remove 'needs rebase' and 'needs work' on new push #51)

  3. =2 CI fails
    -> add "needs work"

  4. IF there are no reviews and >=2 CI pass
    ->add "needs review"

  5. IF there are reviews
    a) still has "needs review" label -> remove it
    b) there's at least one review with changes request -> 'needs work'
    c) otherwise if the last action was before more than a day ago and PR is labeled with neither 'needs {work, decision, extra review} than:
    c1) if there are two are more approval reviews -> 'needs decision'
    c2) otherwise -> 'needs extra review'

  6. On inactivity
    and "needs work": ping contributor (Daily cronjob: first iteration #52)
    and "needs decision": ping reviewers

Note that I replaced "author action needed" with the 'needs work' because it's an already existing label and it fits nicely in the 'needs X' structure.

More feedback

I think point (4) is a bit opportunistic: reviews are usually very rare and case that a PR has two approvals and doesn't get immediately merged almost never happens. It is much more likely that a PR receives a single or two comments and then sink into the PR queue abyss. So in the end it's all just about keeping reviewers happy and motivated ;-)

@WebDrake
Copy link

@wilzbach wow, you have been busy! :-)

It might take me a little while to get to look through this properly. I'll try to give it the attention it deserves before the end of the week.

@andralex andralex assigned andralex and MartinNowak and unassigned andralex Feb 22, 2017
@andralex
Copy link
Member

I see @MartinNowak is on this, leaving it to him. FWIW it seems a more favorable out-of-the-box experience for new contributors would be a net positive.

@wilzbach wilzbach mentioned this pull request Mar 4, 2017
Merged
MartinNowak
MartinNowak approved these changes Mar 25, 2017
View reviewed changes
Copy link
Member

@MartinNowak MartinNowak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A welcome msg seems fine, should contain some guidance, e.g. a link to CONTRIBUTING.md.
Also sounds fine to just always add a comment to begin with.

source/dlangbot/github.d Outdated
{
import std.array : appender;
import std.format : formattedWrite;

auto combined = zip(refs.map!(r => r.id), refs.map!(r => r.fixed), descs.map!(d => d.desc));
auto app = appender!string();
app.formattedWrite("Hi @%s. Thanks a lot for your contribution.", pr.user.login);
app.put("Fix | Bugzilla | Description\n");
app.put("--- | --- | ---\n");
Copy link
Member

Choose a reason for hiding this comment

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

No need for a table header when there are no referenced issues.

@wilzbach
Copy link
Member Author

Just found a link that is probably worth citing: https://wiki.dlang.org/Starting_as_a_Contributor#Create_a_pull_request

@wilzbach wilzbach mentioned this pull request Jun 12, 2017
Merged
@wilzbach wilzbach force-pushed the add-hello-msg branch 2 times, most recently from 9e08b24 to 7014bd2 Compare June 22, 2017 04:32
@wilzbach
Copy link
Member Author

Okay I finally found time to rebase this - as I now have a bit of time to babysit this and we all agree on that adding a "hello" msg is useful, I will merge this now & have a look at the bot.

Of course, the message isn't intended to be fixed, but once its' working we can trivially change it.

@dlang-bot dlang-bot merged commit a986d12 into dlang:master Jun 22, 2017
@wilzbach wilzbach deleted the add-hello-msg branch June 22, 2017 04:49
@wilzbach
Copy link
Member Author

In action:

image

(still needs a bit of tweaking)

This was referenced Jun 22, 2017
Merged
Merged
@andralex
Copy link
Member

Belated congrats for this great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinNowak MartinNowak MartinNowak approved these changes

Assignees

@MartinNowak MartinNowak

Labels
auto-merge question
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wilzbach @WebDrake @MartinNowak @andralex @dlang-bot