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

[Github] Add Pull Request template with links to Github docs/Discord/Forums #71923

Closed
wants to merge 1 commit into from

Conversation

DavidSpickett
Copy link
Collaborator

Inspired by Compiler Explorer's template, which just adds a hidden comment thanking the submitter, which I appreciated a lot when I contributed there.

Also by this post https://discourse.llvm.org/t/how-to-find-reviewers/74803.

This template gives us a way to direct folks who jump directly into making a PR to the help we have already written up.

I often assume (for better or worse) most GitHub hosted projects have similar processes, so if I was new to LLVM I likely wouldn't know about our docs either.

@DavidSpickett
Copy link
Collaborator Author

See https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository for how the template works. From my experience over in Compiler Explorer, it gets added to the end of the text Github initially selects (usually the commit body).

@DavidSpickett DavidSpickett changed the title [llvm][Github] Add Pull Request template with link to Github docs/Discord/Forums [llvm][Github] Add Pull Request template with links to Github docs/Discord/Forums Nov 10, 2023
@DavidSpickett DavidSpickett changed the title [llvm][Github] Add Pull Request template with links to Github docs/Discord/Forums [Github] Add Pull Request template with links to Github docs/Discord/Forums Nov 10, 2023
@DavidSpickett
Copy link
Collaborator Author

Best guess at reviewers, feel free to add more appropriate people.

@kbeyls kbeyls requested a review from tru November 10, 2023 11:48
@kbeyls
Copy link
Collaborator

kbeyls commented Nov 10, 2023

Adding @tru as a reviewer too as I think there's a change he may have experience with this in other github projects.

@kbeyls
Copy link
Collaborator

kbeyls commented Nov 10, 2023

I'm wondering if it would be worthwhile to directly add some info about how to add reviewers here, rather than refer to the docs, given that most newcomers seem to get confused about not being able to set reviewers in the github interface?

Another thought (not sure if useful) is that I've seen in other projects that some add a check-list here, to help people remind what checks they should've done before or shortly after uploading their PR. For LLVM, off the top of my head, I'm thinking of e.g. something somewhat as follows:

  • I considered adding or adapting tests for changed or new functionality.
  • the regressions tests all pass locally on my machine.
  • my patch is properly formatted using e.g. clang-format.
  • I found and added appropriate reviewers on the PR, either by mentioning them in a comment or in the "reviewer" widget.

@DavidSpickett
Copy link
Collaborator Author

I'm wondering if it would be worthwhile to directly add some info about how to add reviewers here, rather than refer to the docs, given that most newcomers seem to get confused about not being able to set reviewers in the github interface?

Sure, the very frequent queries could be included.

Another thought (not sure if useful) is that I've seen in other projects that some add a check-list here, to help people remind what checks they should've done before or shortly after uploading their PR.

My only issue with that is whether we have to remove that text before landing, and who does that. Though it could go in this same comment block, it just wouldn't be interactive. Which is fine if it's a reminder, not something reviewers are gonna require is done.

I'm just wary of adding a manual step that people may forget to do, but also I've not been a merger on a project with one of these checklists either so don't have the answers here.

Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

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

I think this is fine, but I agree with @kbeyls that we should maybe include some of the more common questions we get like "how do I add reviewers" and "how often should I ping the issue"? These seems to be the most common questions we get.

.github/pull_request_template.md Outdated Show resolved Hide resolved
@tru
Copy link
Collaborator

tru commented Nov 10, 2023

Another thought (not sure if useful) is that I've seen in other projects that some add a check-list here, to help people remind what checks they should've done before or shortly after uploading their PR. For LLVM, off the top of my head, I'm thinking of e.g. something somewhat as follows:

My experience with the checklists are that most people ignore them. But I don't mind them being there, we just need to make sure that it's updated if we change the documentation to suggest something other.

@DavidSpickett
Copy link
Collaborator Author

Kristof suggests experimenting with the checklist on an llvm fork, so I will do that.

Inspired by Compiler Explorer's template, which just adds a hidden
comment thanking the submitter, which I appreciated a lot when I
contributed there.

Also by this post https://discourse.llvm.org/t/how-to-find-reviewers/74803.

This template gives us a way to direct folks who jump directly
into making a PR to the help we have already written up.

I often assume (for better or worse) most GitHub hosted projects
have similar processes, so if I was new to LLVM I likely
wouldn't know about our docs either.
@DavidSpickett
Copy link
Collaborator Author

Added how to get reviewers and how to ping based on the text in https://llvm.org/docs/Contributing.html#how-to-submit-a-patch.

@DavidSpickett
Copy link
Collaborator Author

If it's ok, maybe this can be reviewed as is, and I will investigate checklists for potential future changes.

@tstellar
Copy link
Collaborator

Will this hidden comment end up in the commit message?

@DavidSpickett
Copy link
Collaborator Author

Will this hidden comment end up in the commit message?

I'll check this on a fork. It's possible that a merge from the command line would include it by default.

@DavidSpickett
Copy link
Collaborator Author

If this is the case, maybe we can use the same text as an llvmbot comment for PRs from new users (istr Github itself knows who is a new contributor).

@tstellar
Copy link
Collaborator

Will this hidden comment end up in the commit message?

I'll check this on a fork. It's possible that a merge from the command line would include it by default.

You should also test a 'squash and merge' from the UI.

@DavidSpickett
Copy link
Collaborator Author

On the assumption that llvm-project has changed the default squash and merge to "Default to pull request title and description" (which matches what I've seen), all the text from the PR template is included however you merge it. There is no way to automatically remove it.

(https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests)

So doing this as a PR template isn't going to work, and on second thought is not the best place for this information anyway. I will look into this being a bot comment for new users instead (or anyone without write permissions?).

@tstellar do you know where llvm-bot's code is? Is it possible to run an instance of it on your own fork?

@kbeyls
Copy link
Collaborator

kbeyls commented Nov 10, 2023

I was thinking that the check list at least might be useful for more than just new users.
I think it would also be useful for occasional users. Or maybe even for regular contributors.
I would be surprised if it would not result in better quality PRs, resulting in slightly less pressure on reviewers to point out easy to fix issues.

So, I think I wouldn't try too hard to only add this for new users, assuming it also has useful information for occasional, or even frequent, PR creators.

Having a bot create such a comment may well provide at least half the value of what's intended to be achieved by llvm/Community.o#7.

@DavidSpickett
Copy link
Collaborator Author

True, I'm already over scoping it :)

Perhaps some of this can be done using the comment llvmbot already leaves on PRs when it labels them.

@DavidSpickett
Copy link
Collaborator Author

@tstellar do you know where llvm-bot's code is? Is it possible to run an instance of it on your own fork?

I found it in llvm/utils/git/github-automation.py.

@tstellar
Copy link
Collaborator

@tstellar do you know where llvm-bot's code is? Is it possible to run an instance of it on your own fork?

You don't need to use the llvmbot account for this. You can just write a simple workflow that adds a comment to each new PR using the builtin GITHUB_TOKEN for authentication.

@DavidSpickett
Copy link
Collaborator Author

Done as a workflow in #72249.

@DavidSpickett DavidSpickett deleted the llvm-pr-template branch November 14, 2023 12:43
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