Alternative, concise PR template #2945
Conversation
This PR proposes an alternative PR template that is much shorter, and removes all the redundant claims. It also links to the contribution guidelines for further guidance.
.github/pull_request_template.md
Outdated
| - [ ] 🧪 Added/updated tests with good coverage | ||
| - [ ] 💡 Added comments for complex logic | ||
| - [ ] ⚙️ Tested my changes with `./gradlew clean spotlessApply build` | ||
| - [ ] 🧾 Updated `CHANGELOG.md` |
There was a problem hiding this comment.
nit: "Updated CHANGELOG.md if the change significantly affects users".... I believe internal changes / refactorings do not need to be mentioned in CHANGELOG.md
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
.github/pull_request_template.md
Outdated
| - [ ] 🔗 Related issue(s): Fixes # | ||
| - [ ] 🧪 Added/updated tests with good coverage | ||
| - [ ] 💡 Added comments for complex logic | ||
| - [ ] ⚙️ Tested my changes with `./gradlew clean spotlessApply build` |
There was a problem hiding this comment.
It might be preferable to mention this in the contribution guide (if not there already). CI will take care of that.... and I personally often prefer CI over complete local tests for small changes 😅
There was a problem hiding this comment.
Maybe suggest spotlessApply check in this case? We all often forget things like formatting or license headers.
There was a problem hiding this comment.
My point being: when someone opens a PR, several people get notified. It's a bit frustrating to be notified on a PR that doesn't even compile or pass the rat tests. I hope this TODO item could save us a few premature notifications :-)
There was a problem hiding this comment.
Contributors with than one PR get automated CI runs. First-time contributors do not get CI automatically, but I guess we need to balance the overhead of reviewing first-time contributions against the main stream of PRs from regular contributors. IMHO, having this checkbox is more of an irritation to regular contributors and I'm not convinced it is going to be effective in making first-timers actually run full tests 😅
In any case, I'm ok both the the current checkbox and with spotlessApply check.
There was a problem hiding this comment.
A relatively quick sanity check is ./gradlew spotlessApply compileAll jar, maybe with the --continue option.
But I'm open to other solution and don't wanna hold this PR. It's often quite opinionated. We can certainly nit-pick on this item for a long time ;)
.github/pull_request_template.md
Outdated
| - [ ] 🔗 Related issue(s): Fixes # | ||
| - [ ] 🧪 Added/updated tests with good coverage | ||
| - [ ] 💡 Added comments for complex logic | ||
| - [ ] ⚙️ Tested my changes with `./gradlew spotlessApply check` |
There was a problem hiding this comment.
| - [ ] ⚙️ Tested my changes with `./gradlew spotlessApply check` | |
| - [ ] ⚙️ Tested my changes with `./gradlew build --continue` |
build exercises the whole build and all checks, including spotless checks (a failed spotless check says: "run spotlessApply").
--continue means to not stop after the first error.
There was a problem hiding this comment.
a failed spotless check says: "run spotlessApply"
So, isn't it better to proactively run spotlessApply?
There was a problem hiding this comment.
It will fail on the CI anyway.
There was a problem hiding this comment.
Agreed with @jbonofre, the style issue failed with CI anyway. I'm OK without it.
.github/pull_request_template.md
Outdated
|
|
||
| ## Checklist | ||
| - [ ] 🛡️ Don't disclose security issues! (contact [email protected]) | ||
| - [ ] 🔗 Related issue(s): Fixes # |
There was a problem hiding this comment.
| - [ ] 🔗 Related issue(s): Fixes # | |
| - [ ] 🔗 Related issue(s): Add `Fixes #` as a separate line |
There was a problem hiding this comment.
Does it have to be on a separate line? I thought GitHub could find occurrences of Fixes # everywhere in the PR description.
There was a problem hiding this comment.
That's right, it could be in any place, including in the PR title.
There was a problem hiding this comment.
Thanks a lot @adutra for working on the simplification!
I think it's quite valuable to ask for "Why are the changes needed?".
This item cover some cases: 🔗 Related issue(s): Fixes #
Maybe we should do something like
- Clearly explained why the changes are needed, or linked the related issues: Fixes #
.github/pull_request_template.md
Outdated
|
|
||
| ## Checklist | ||
| - [ ] 🛡️ Don't disclose security issues! (contact [email protected]) | ||
| - [ ] 🔗 Related issue(s): Fixes # |
There was a problem hiding this comment.
This means every PR needs an issue ? The issue would contain motivation etc ?
.github/pull_request_template.md
Outdated
| ## Checklist | ||
| - [ ] 🛡️ Don't disclose security issues! (contact [email protected]) | ||
| - [ ] 🔗 Related issue(s): Fixes # | ||
| - [ ] 🧪 Added/updated tests with good coverage |
There was a problem hiding this comment.
[doubt] what does good means ? how is the user gonna describe manual testing done ?
.github/pull_request_template.md
Outdated
| - [ ] 🔗 Related issue(s): Fixes # | ||
| - [ ] 🧪 Added/updated tests with good coverage | ||
| - [ ] 💡 Added comments for complex logic | ||
| - [ ] ⚙️ Tested my changes with `./gradlew spotlessApply check` |
There was a problem hiding this comment.
[doubt] is this adding any additional value ? wouldn't the CI fail without this ?
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thank you all for your the reviews! |
6 participants
This PR proposes an alternative PR template that is much shorter, and removes all the redundant claims.
It also links to the contribution guidelines for further guidance.
Checklist
./gradlew clean spotlessApply buildCHANGELOG.mdsite/content/in-dev/unreleased)