-
Notifications
You must be signed in to change notification settings - Fork 34
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 more PULL_REQUEST_TEMPLATE instructions #48
Conversation
To help people finish everything before pinging a PR for its final review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make people check that the build is passing. Maybe for now we can add that to the comment? Commit message makes sense though as that's often forgotten and actually somewhat time-consuming to construct yourself.
PULL_REQUEST_TEMPLATE.md.template
Outdated
- [ ] [Implementation bugs](https://github.com/whatwg/meta/blob/main/MAINTAINERS.md#handling-pull-requests) are filed: | ||
* Chromium: … | ||
* Gecko: … | ||
* WebKit: …@@extra_implementers@@ | ||
- [ ] [MDN issue](https://github.com/whatwg/meta/blob/main/MAINTAINERS.md#handling-pull-requests) is filed: … | ||
- [ ] There is a clear commit message to use when squashing this PR. Either squash the commits yourself or write the commit message here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [ ] There is a clear commit message to use when squashing this PR. Either squash the commits yourself or write the commit message here: | |
- [ ] There is a [clear commit message](https://github.com/whatwg/meta/blob/main/COMMITTING.md) to use.<!-- Editing the commit message above is okay, but beware of race conditions with the PR Preview bot. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comment you've suggested: isn't it ok to edit everything in the initial comment, for example to continue filing in implementation bugs? Where's "above"? And PR-Preview will automatically rebuild after every edit, so I've never had a problem with races.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you create a PR the initial commit message is usually included above this checklist. Editing that would be fine.
If you change the top-level comment directly after you create a PR you can definitely get into a race with PR Preview where it overrides your changes. See for instance the history of OP of whatwg/fetch#1434. PR Preview overrides a change that was made and nobody noticed afterwards (until now, as I was trying to find an example).
As for what should be in the list, it's a trade off between ensuring things are in order and making it more difficult to contribute when things are in order. I guess I'd rather start out with conveying expectations in writing and then moving to a checkbox later if that's not successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks. I've updated this PR again to try to capture that.
I'm not 100% tied to including "The build is passing" in the checklist, but I think that if you or other editors are going to be annoyed when something isn't done, it should be in the list. We could say something like "Ask if you need help with this." if there are build problems that people have lots of trouble fixing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great!
To help people finish everything before pinging a PR for its final review.
This is based on some things Anne mentioned at https://matrix.to/#/!AGetWbsMpFPdSgUrbs:matrix.org/$eRoNst7XvM_u7lC03XyD44vEd8AwR029JvvrA7BDxXQ?via=matrix.org&via=mozilla.org&via=igalia.com. I'm not tied to any particular expression of these ideas and would be fine with being told that this isn't the right place to give these instructions.