enrich existing PR template to smooth the review process #1264
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1264 +/- ##
==========================================
- Coverage 20.90% 20.14% -0.76%
==========================================
Files 196 203 +7
Lines 25425 26824 +1399
==========================================
+ Hits 5316 5405 +89
- Misses 19118 20412 +1294
- Partials 991 1007 +16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @p0mvn , thanks for reviewing and suggesting ideas!
re: checkbox, background on why I'd prefer trying not having checkbox at least first is:
- as mentioned in enrich existing PR template to smooth the review process #1261, one goal is to avoid the excessive checkboxes in template like cosmosSDK's, which is unreadable and resulting in people just completely ignore them, like 1 or 2
- this template proposal is mostly borrowed from Apache foundation projects where I've been heavily involved for the past few years, and it has been proven works well at scale of thousand of contributors.
All being said, we can always see how it goes then iterate over it, and this should not be a final version we settle on. More than happy to iterate based on feedback, but highly encourage us to give the proposed format a try first and further polish it down the road.
|
Appreciate all the clarifications @jblbit , I saw your example PR descriptions such as #1268 which looks great. I see the value in having to encourage everyone to put more thought into the descriptions by having fewer checkboxes. Open to trying that first
I think the major issue with people not utilizing the template in the SDK is stemming from the fact that the team is not encouraging its use. If we want to encourage all contributors to use our template, we should ultimately start reminding each other to fill in the description in every PR and not ignore it |
There was a problem hiding this comment.
This is awesome! Yeah I agree that cosmos sdk's PR template and our current template with all the check box is easy to ignore.
However, I don't know if I quite agree that we should change it to the way an user answers yes or no to a question as this PR proposes (e.g - Is PR targeted against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/gaia/blob/main/CONTRIBUTING.md#pr-targeting)) (yes / no))
I don't have strong opinions for this though and would follow along with what others think, but my suggested change would be keeping this sorting using the headers(e.g) what is the purpose of the change, brief change log etc) and then keeping a checkbox underneath it.
|
I think part of the problem with checkboxes, is they don't work too well in the PR creation template. (You have to manually type x, or do a thing after hitting submit) |
There was a problem hiding this comment.
Love the motivation for this! I'm a bit hesitant because it makes the template more verbose for the PR author. Can we find a happy medium similar to what we do for the SDK?
Essentially, it's a checklist, one for the PR author and one for the repo maintainers. You can find it here: https://github.com/cosmos/cosmos-sdk/blob/master/.github/PULL_REQUEST_TEMPLATE/other.md
|
|
||
| ## Documentation and Release Note | ||
|
|
||
| - Does this pull request introduce a new feature or user-facing behavior changes? (yes / no) |
There was a problem hiding this comment.
Can this entire section be converted to a checklist? Similar to what we do for the SDK.
.github/pull_request_template.md
Outdated
|
|
||
|
|
||
| - Is PR targeted against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/gaia/blob/main/CONTRIBUTING.md#pr-targeting)) (yes / no) |
There was a problem hiding this comment.
This should also be added to the checklist below.
|
Hi @alexanderbez !
Good question. To clarify, it's not aimed to save time for PR author, but for PR reviewers. So please evaluate it from a reviewer angle, e.g. would you be able to review someone else's PR with this template more efficiently and confidently, even with little context? Honestly authors should take the extra work to make it easy to review by others, and don't count on reviewers to check for them.
As mentioned here, we discussed and wanted to try sth new rather than what cosmosSDK does. I suspect why the PR review process in cosmosSDK is not effective is mostly counting too much on reviewers rather than authors, and also checkbox in practice don't work as most expected. |
|
Hi all, based on feedback, I'm gonna take out the part of "Is PR targeted against correct branch" which is inherited. This may just be a bylaw and no need to check for every PR. BTW, Roman opened a PR with this new template as a good example. It makes it much easier at least for me to ramp up and review, compared to most other opened PRs. All being said, understand it's a bit different from what we have so far, but I highly recommend trying it which has been proven at scale. We can always make changes based on real usage feedback afterwards :) |
|
I'm down to try this and iterate on it! I actually like the non-checkbox system, and want to try it out at least. I feel like I've been in projects using checkboxes for PRs for years now, (not just in Cosmos), and none have had great success with getting contributors to follow the checkboxes. (in part, because imo the markdown syntax is confusing in preview) We may want to cut down on the intro blurb so more of the relevant text automatically fills in, but I think that will become more apparent as we do more PR's and iterate on the templtae |
|
Yeah totally down to at least try it as well! |
|
Hi all, thank you for trying it out！Love to hear your feedback in a few weeks. Can you help merge it, or grant me write access to merge it? btw, in order to see it takes effect, all of us will be leading the change here, as nothing comes for granted :) e.g. we have to be the role model to follow it ourselves; if someone is not following it, please politely point it out and ask them to reformat. |
|
I'm highly skeptical PR authors will fill this out in detail, but I suppose it can't hurt to try. |
Closes: #1261
NOTE! I‘m using the proposed PR template for this PR, to demonstrate how it can work
What is the purpose of the change
Adding a bit more templating to help smooth the review process, as well as fostering a engineering habit of emphasizing documentations and tests.
Brief change log
PR Formatting
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (no)