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

Improve pull request review experience by tagging them based on size and contents #5402

Closed
wants to merge 3 commits into from
Closed

Conversation

orf
Copy link
Contributor

@orf orf commented Oct 7, 2021

This PR adds two workflows. The first one tags new pull requests based on their size and the files changed. This action attempts to programmatically segment new pull requests into several overlapping buckets:

  1. Test only changes
  2. Documentation only changes
  3. Tiny changes (<10 lines)
  4. Small changes (<25 lines)

You can see it in action here: https://github.com/orf/distributed/pull/7. As the pull request is updated the tags also are updated to reflect the contents.

Why? In any project like this reviewer time is limited. We can use the size of the pull request as a proxy for how long it will take to review - it's not perfect, but I think it's a good heuristic to use. In a similar vein, PRs that only impact tests and docs likely require less discussion or are possibly easier to review - a tiny PR that only touches tests is usually pretty simple to review and merge. With this people can now see at a glance which PRs they might be able to look at in a given time frame.

It's also fairly easy to add new tags based on the files changed, for example we could add dashboard, protocol or diagnostics labels to indicate changes isolated to those modules.

The second action closes stale pull requests. A large percentage of the open pull requests are over a year old, and it's probably a good thing to mark them as stale and close them.

@fjetter
Copy link
Member

fjetter commented Oct 8, 2021

Thanks for putting this together. There has been some chat about this in dask/community#60 but I believe this never reached a conclusion.

For reference, a few people are currently organizing a manual walk-through, see dask/community#188

@GenevieveBuckley
Copy link
Contributor

I really like the idea of auto-labelling of PRs based on what files have changed. @jsignell would you find something like this helpful in dask/dask as well?

I don't like the idea of the stale bot that automatically closes issues/PRs. From the other conversations that have been happening, it seems the consensus is it's good to have a bot to label old issues, so that a human maintainer can give them more attention. Auto-closing is generally a negative experience for users/contributors. (Based on the title I didn't realize this PR introduced a stale bot action until after I made #5405 - sorry about that)

@orf
Copy link
Contributor Author

orf commented Oct 12, 2021

I agree that closing issues is really annoying and counterproductive, but a large portion of the pull requests here are clearly abandoned. We could indeed label them, but is there any value in having a year+ old merge request full of conflicts open? We're not deleting them, we're only closing them.

I agree we should label old issues for review. Happy to remove the stale PR closing if that’s contentious

@orf
Copy link
Contributor Author

orf commented Oct 17, 2021

I've removed the action that closes stale pull requests, shall we merge the tagging one and see how it goes? We can expand the labels in the future as well.

@GenevieveBuckley
Copy link
Contributor

ping @dask/maintenance

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @orf. While I appreciate the thought you've put into this and generally and am in favor of reducing the burden on maintainers, I don't think the small /tiny labels will be that useful for most folks who review distributed PRs relative to the complexity introduced in this workflow.

For the test-only / docs-only labels, I recommend using GitHub's built-in labeling system similar to what we do in dask/dask

@orf orf closed this by deleting the head repository Aug 5, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants