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

Add pull request template #4919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rkoeze
Copy link

@rkoeze rkoeze commented Jun 22, 2024

Adds a PR template to encourage standard descriptions as discussed in #4912 (comment).

@rkoeze rkoeze force-pushed the rkoeze/add-github-pr-template branch 3 times, most recently from b89b4ad to 7408795 Compare June 22, 2024 17:38
@rkoeze rkoeze marked this pull request as ready for review June 22, 2024 17:38
@tomhughes
Copy link
Member

Does that template really address the things @gravitystorm was talking about? It doesn't seem like it does to me but then I'm not sure a PR template can which is why I was a bit confused by the suggestion - the automation he was suggesting seems more likely to work.

I'm certainly not convinced that asking people to assess the risk of a change is going to be useful - risk of what exactly? If somebody truly thought a change was risky then they shouldn't really be proposing it!

@rkoeze
Copy link
Author

rkoeze commented Jun 22, 2024

Hi @tomhughes! I think there's probably a version of a PR template that does address what @gravitystorm was talking about (most likely in the form of a checklist). This template does not try to do that. It's aim is to enforce a consistent PR description. If that doesn't make sense for this repo, I'm happy to close this PR. After all, this change is meant to make the lives of reviewers easier!

I can definitely remove the risk section. I've found that in other projects asking for a risk assessment can be a useful exercise for the contributor. It can also give the reviewer helpful context when testing. A worst case idea of "what could go wrong here" is what I mean by risk.

Thank you for your feedback.

Copy link
Contributor

@tordans tordans 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 a good template that will make thinks better for some PRs. I certainly does not harm the process, because one can always remove all this very easily when creating a PR that does not fit this schema.

I would also remove the risk section. And make the intro even more explicit on the PR section which is where all the "do squash, do rebase, no merge commits" part is written.

@@ -0,0 +1,10 @@
<!--Please read the contributing guidelines before making a PR: https://github.com/openstreetmap/openstreetmap-website/blob/master/CONTRIBUTING.md-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!--Please read the contributing guidelines before making a PR: https://github.com/openstreetmap/openstreetmap-website/blob/master/CONTRIBUTING.md-->
<!--
Please read the contributing guidelines before making a PR:
https://github.com/openstreetmap/openstreetmap-website/blob/master/CONTRIBUTING.md
Especially the section on how to present PRs:
https://github.com/openstreetmap/openstreetmap-website/blob/master/CONTRIBUTING.md#pull-requests
-->

Comment on lines 8 to 10

### Risks
<!--Is this a low, medium, or high risk change? Explain why.-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Risks
<!--Is this a low, medium, or high risk change? Explain why.-->

@rkoeze rkoeze force-pushed the rkoeze/add-github-pr-template branch from 7408795 to 31da85b Compare June 23, 2024 12:54
@rkoeze rkoeze force-pushed the rkoeze/add-github-pr-template branch from 31da85b to e279458 Compare June 23, 2024 12:55
@rkoeze
Copy link
Author

rkoeze commented Jun 23, 2024

Thanks @tordans! I removed the risk section and added the link to the PR section of the contributing guidelines as you suggested.

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

3 participants