-
Notifications
You must be signed in to change notification settings - Fork 54
Propose/Create PULL_REQUEST_TEMPLATE.md #935
Conversation
This patch proposes a Pull Request Template so that each PR can have a thorough description. Please feel free to discuss the content.
I think we already have this under developer guidelines (docs/developer-guidelines.md), perhaps you could update those? |
@naved001 as discussed |
Just in case you didn't what this does (like me a few minutes ago), this is what @xuhang57 is doing https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ |
PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] Add unit tests in the corresponding file and create one if none are present. | ||
- [ ] If practical, bug fixes should have an reproducing test to ensure that the bug does not come back. | ||
- [ ] Run deployment tests if code could affect switches | ||
- [ ] Reviewed and approved by at least one other contributor. |
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.
We require at least 2 reviews per PR, or one review for simple documentation changes.
as Naved mentioned, requires 2 for most PRs
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.
Two minor things.
In addition, I think there's a way to set a contribution guidelines file, which Github will prompt the user to read. Since most of this is a checklist, that may make more sense; we don't really want the checklist itself in the PR description I think. @naved001, thoughts?
PULL_REQUEST_TEMPLATE.md
Outdated
@@ -0,0 +1,15 @@ | |||
## Description | |||
|
|||
Please describe your pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue. When linking to an issue, please use `refs #...` in the description of the pull request. |
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.
Not sure I fully understand the point of specifically using refs #...
? What's wrong with Github's #...
notation (or just a link, which I think Github will still figure out)?
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.
oh yes, Github's #...
should work :)
PULL_REQUEST_TEMPLATE.md
Outdated
|
||
## Checklist before merging Pull Requests | ||
|
||
- [ ] If functionality could have architectural implications or controversial, have a discussion with the team. Ideally, prior to coding to save effort. |
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.
Maybe add something point to the specs process for large changes.
I believe it is possible to have contribution guidelines in such as: And maybe not having such a long checklist but having a simple checklist in the PR imho could save time for both contributors and reviewers ;) |
https://help.github.com/articles/setting-guidelines-for-repository-contributors/ maybe this works But I still think these two features serve two different purposes. What do you guys think? |
All it says is to describe what their PR is doing and to refer to the issue, and whoever has read* the github docs would know how to do that hopefully. I think the contribution guidelines page is good enough (since it will display a prompt on top when submitting a PR) and this might not be necessary. |
I agree pull request templates serve a somewhat different purpose than the contributing doc, but I also don't think we need a pr template as-is; the message directing folks to read the contribution guidelines seems like it should cover what's here currently. |
Github feature:
This patch proposes a Pull Request Template so that each PR can have a thorough description. Please feel free to discuss the content.