pr template changes - #2404
Conversation
| ### Breaking Changes? | ||
| - [ ] other | ||
|
|
||
| Are tests included? |
There was a problem hiding this comment.
There are valid reasons not to include tests. CI updates, config updates, README updates, etc. I understand the intent here, but it's not accurate.
| - [ ] feature | ||
| - [ ] refactor | ||
| - [ ] tests | ||
| - [ ] refactoring |
There was a problem hiding this comment.
I believe "refactor" is correct here, as the verb describes the state. The past form is "refactored," while the present participle "refactoring" describes something currently in process, the action of undertaking a refactor. While a PR could be considered in process, it's supposed to be a finished work ready for review and merging. Don't be to be pedantic about it, I just see these things and they stand out to me 😄
There was a problem hiding this comment.
Caught my eye too, because refactor is a verb, when this is a list of nouns. refactor could be argued as a noun I guess, but refactoring feels more like the right noun to me. Definitely personal preference though.
There was a problem hiding this comment.
Only mentioning because English is fun (seriously, I get a kick out of this stuff). bugfix, feature, and refactor are all part of a class of words called "gerunds," or verbs that are used as nouns.
| - [ ] yes | ||
|
|
||
| Breaking Changes? | ||
| - [ ] yes (please describe clearly below) |
There was a problem hiding this comment.
since we're removing the heading, let's make (please describe clearly below) stand out some more by bolding it. emphasis here is a good thing.
lukastaegert
left a comment
There was a problem hiding this comment.
I generally like the changes but I am not 100% sold on not giving breaking changes their own line. It might make sense to move it to the bottom, though i.e. leave everything as it is in the PR but add another heading at the bottom to the tune of "If applicable, what are the breaking changes exactly?"
The reason is that it for the few PRs I opened since we have the template, I felt this line helped me focus and formulate clearly what is going to break. Which again will help us to make sure, no necessary documentation updates or deprecations are forgotten. Otherwise I fear it might get spread all over "Describe your changes".
|
@lukastaegert one of the reasons I created this was actually exactly to describe what you are asking - to make it clear breaking changes should not get their own section but be included in the main description. (which was my impression on first read of the original) I've altered the wording slightly here. Let me know if you have any suggestions for clarification further. |
|
I've also altered the PR description title - otherwise it seems a bit odd I think having all PRs reading "Please describe your changes" as their heading. Other suggestions welcome too. |
I'd like to suggest some minor adjustments to the PR template.
I'm also wondering about the "lint" one - do we need this with the commit hook? @lukastaegert