update pull request template#456
Conversation
oelbert
left a comment
There was a problem hiding this comment.
The style guidelines are more than just what's in the linter and include things like naming conventions, so I would keep that in the checklist. Otherwise looks good, thanks for updating this!
|
|
||
| ## Checklist | ||
|
|
||
| - [ ] My code follows the style guidelines of this project |
There was a problem hiding this comment.
I actually would keep this, as the style guidelines encompass more than just linting
There was a problem hiding this comment.
I'd like to argue this one. In my opinion, a PR checklist should be self contained in the sense that developers can do something about each point. To the best of my knowledge, there are no "style guidelines of this project" other than "please run the linter", which run automatically. Everything else is not written down anywhere and will naturally come up in code reviews if reviewer think it's worth debating about.
I am happy to re-introduce this line once there is a style guide that I can link from the checklist item (such that (new) developers can go and do something (i.e. read up) on their own). For now, I think we have a very small team of people contributing and - in my experience - debates about naming conventions and other implied style issues almost never come up in practice.
Description
There are two small things I'd like to update in the PR template:
Reasoning We enforce "the style guidelines of this project" with linting. That process is automatic and thus the checkbox is always checked. I don't see a need for it anymore.
Reasoning The release (and patch release) process are described in this internal documentation. Per se, a PR has nothing to do with whether or not a (patch) release is made.
How has this been tested?
Any potential concerns should be discussed in the NASA/NOAA sync on Friday.
Checklist