Clean up & shorten PR template - #120259
Conversation
31b124b to
b496750
Compare
| --> | ||
|
|
||
| ## What problem(s) does this PR solve? | ||
| ## What issue(s) or proposal does this PR address? |
There was a problem hiding this comment.
I was thinking it’s uncommon that a PR addresses multiple proposals.
There was a problem hiding this comment.
It does happen more than you'd think, particularly when there's duplicate or overlapping proposals.
allenwp
left a comment
There was a problem hiding this comment.
There are parts of this that I agree with and think are a good improvement. Thanks!
I removed the unnecessary explaining of why master needs to be targeted. That info could be (should be) found under the linked page; the rule is simple, and the explanation doesn't need to be there.
I agree with this and think the implementation is good. I agree there is benefit to shrinking lines 2 through 4 to this new single line.
I changed the wording so that PR's aren't just about fixing problems, but also about addressing proposals.
The intent behind master's "problem(s)" wording is twofold:
- It frames the PR in a "problem-first" or "problem oriented" paradigm, which encourages both authors and reviewers to think about this PR in terms of the problem(s) that it solves. This is about getting all folks who are involved in PR authoring and review into the Godot mindset. Review meetings have demonstrated that even seasoned Godot maintainers sometimes need a reminder to use this framing/perspective when reviewing PRs.
- Creating GitHub issues or proposals for PRs is not strictly required by the Godot project; there are a fairly large number of very good and valid small bugfix PRs that do not have associated GitHub issues or proposals.
master's heading is better suited for free-form text that describes small problems that don't need a related GitHub issue.
To make this heading fit better with the two other headings in this PR, we could change it to be ## Problem(s) solved by this PR instead of the current question-like wording. The question-like wording is a bit of a holdover from a time where each heading would ask a question of the author; with the other two headings not being questions, it prompts revisiting this wording. But this is a super minor style thing. I don't care much about this nitpick on master's wording.
I removed the second link because nobody is going to check a link for examples, and if they want to, then perhaps that info should be part of the first link?
My perspective is that we should have well-meaning new contributors who are putting effort and thought into their PRs as our primary target audience. Including this link helps these contributors to submit their best work. I believe those who don't bother visiting the relevant links that they are directed to are a secondary audience that should be targeted in ways that do not hinder the primary target audience.
I believe that removing this link that specifically describes how to write a good "Additional Info" section could hinder our primary target audience from doing their best work, even though it may make things slightly better for the secondary audience that doesn't bother looking at linked documentation.
I've included one other comment in this review. Thanks for putting this together!
b496750 to
89b286e
Compare
|
Added back the second link as argued by @allenwp and brought back the 'Problem(s)'. (haha) |
allenwp
left a comment
There was a problem hiding this comment.
Thanks for the updates, this looks good to me!
I personally see this as an overall improvement. Some of the changes are just minor style changes, so I expect no difference in contributor behaviour in practice.
The Author disclosure section is something I can't really comment on because I haven't yet been exposed to dealing with PRs that have been created with AI tooling.
One final note is that the docs should probably do a better job of communicating how fixes will be backported to older branches and versions... With this new PR I would expect this note to be in the checklist (since that's linked right after the text that says to target master) or in one of the pages that's directly linked from that docs section. But this can be handled in a separate contributing docs PR.
Problem(s) solved by this PR
The current PR template is a bit verbose. I've heard many contributors say they dislike it, despite understanding why it's there. To weaken that ever-so-strong desire to want to
Ctrl+Aanddeletethe whole thing, this PR is an attempt to make it more succinct while keeping the most meaningful / guiding / educational info present.Additional information
https://godotengine.org/pr-checklist?Author disclosure
No generative AI was used for this PR.