Require listing solved problems in PR descriptions and add links and instructions.

[allenwp]

What problem(s) does this PR solve?

1. During PR review, especially in team meetings, it can take too much time to understand the root problem that a PR aims to solve.
2. During PR review, it is sometimes difficult to understand the rationale that was used for changes. This is especially a problem when an incorrect assumption is made by the reviewer regarding why a change was necessary.
3. PR authors who have not recently read the updated contributing docs may be unaware that they must now disclose their use of AI tools.
4. PR authors may assume that PR review will be extremely thorough and will catch any mistakes or sub-optimal decisions that were made in the PR, even when the PR author is aware of areas that may have mistakes or sub-optimal decisions. But, in practice, reviewers are left unaware of what specific parts of a PR might need special attention, making the review process more difficult and sometimes more error prone.
5. PR authors may desire and assume and that a review will initiate discussion over a technical detail that is relevant to the review, but the discussion will never happen because the technical detail was never brought up.
6. Non-new contributors are not prompted to periodically re-read the checklist for new contributors.

Additional information

This PR is a second attempt that supersedes #118624 with a different approach based on feedback from the first PR.

Problem 1: It can take too much time to understand the root problem

Problem 1 is solved by effectively requiring PR authors to list the problems that their PR solves. This reinforces the Godot way of making changes: The problem always comes first. By having this template at the top of the PR description, it may become faster for reviewers to understand the PR in the ways that are necessary for a good and thorough review.

As an anecdote: After attending many core and rendering team meetings, I have found that it is somewhat common for PR review to spend 10 or 15 minutes discussing a PR before Clay finally steps in and asks "What problem is this PR even aiming to solve?" and then we realize immediately that all of our talk was pointless because the PR doesn't do a good job of solving that problem or tries to do more than is necessary to actually solve this problem. My goal with this change to the PR template is that this waste of time will be avoided because all reviewers will be brought up to speed on what problem a PR solves as the first step in the review process.

Problem 2: It is difficult to understand the rationale that was used for changes.

Problem 2 is solved by prompting and reminding PR authors to provide additional information and explanation. Although these guidelines exist in the contributing docs, PR authors will be more likely to review them regularly as a reminder of things that may be appropriate for their PR description when this page is linked from the PR template.

Problem 3: PR authors may be unaware that they must disclose their use of AI

Use of AI for programming has become extremely normalized in some industries, so I believe adding a one line reminder to PR authors will be useful in helping contributors not get caught off-guard by this relatively new requirement.

Problems 4 and 5: PR authors may assume that PR review will be extremely thorough and initiate discussion over a technical detail

This PR prompts PR authors to consider writing additional information and explanation about "areas that you are uncertain of or require special attention from reviewers". By including this specific prompt, it helps reviewers understand specific areas that may need extra attention and reduces the risk of bugs being introduced due to a large PR including changes that the original author was uncertain of.

Problem 6: Non-new contributors are not prompted to periodically re-read the checklist

Although the checklist for new contributors is titled for "new" contributors, it includes information that is changed periodically and must be reviewed by all contributors, both old or new. By removing the "new" word from the existing comment text, it helps remind existing contributors to periodically review this checklist.

Personally, I have fallen into this trap: because it is a checklist for "new" contributors, I haven't read it for years because I assumed it was just about setting up local style checks and that sort of thing. But it's not: it also includes other important reminders that all contributors should act on.

Discussion

Originally I had the new "What problem(s) does this PR solve?" section written as follows:

### What problem(s) does this PR solve?

<!--
If the problems are described in an existing issue or proposal, you may link to them in the following list and use closing keywords if this PR closes them: https://docs.github.com/articles/closing-issues-using-keywords

Read our best practices for engine contributors page to learn more about our problem-first approach to development: https://contributing.godotengine.org/en/latest/engine/guidelines/best_practices.html
-->

1.

But this approach was simply too much text for the template, in my opinion. I removed the comment about using closing keywords and simply put the closing keyword in the problem list as suggested by charjr

[Ivorforce]

I prefer this form of the template.

I agree that a 'what problems does this solve' is always needed.

The link to the "here's other ideas what you can add" also makes sense to me.
Depending on how bad it gets with AI, we could expand the AI thing into its own section, but a quick comment above is a decent start.

I think i'd be OK with merging this, but we need more discussion / eyes on it first.

[Ivorforce]

Personally, I have fallen into this trap: because it is a checklist for "new" contributors, I haven't read it for years because I assumed it was just about setting up local style checks and that sort of thing. But it's not: it also includes other important reminders that all contributors should act on.

By the way, it hasn't existed for years :) it was just added a few months ago, partially to have a place to put the "please disclose AI" phrase.

[allenwp]

Hah! Well, I hadn’t read the rest of the contributing guidelines for years, so I just assumed this list was a part of what did exist and glossed over it for the last few months thinking it was just for new contributors.

[charjr]

Regardless of whether more is needed, I think this would be an improvement to the existing template.

[Ivorforce]

Thinking about this again, I definitely think it's worth a try, so I want to voice my support for merging this (though we still need more opinions).

If it ends up not helping much and/or annoying contributors, we can always revert it.

[Ivorforce]

In the same line as Qbie's comment above, it might be worth changing the title of this to something more neutral like Description. That way it might feel less like "By providing issue links, you've done enough - if you feel motivated, add more here" and more like two important parts of the template.

[allenwp]

I believe a short description of the PR is already covered by the commit text that’s included as first lines of the PR and the PR’s title. This is part of why I left this section as intentionally “Additional information” instead of “Description”.

Also, to me, “Description” is a misleading heading that does not well describe the intent of this section: Problem numbers 2, 4, and 5 are much better solved with an “Additional information” heading than with a “Description” heading; although I could make it work, it would not be intuitive for me to put the information required to solve problems 2, 4, and 5 under a “Description” heading.

I think it’s best to avoid accidentally encouraging PR authors to simply describe the changes of the PR because this is duplicate information; the “files changed” section of a PR already perfectly and accurately describes the changes of the PR. Additionally, I believe new contributors are sometimes inclined to simply describe the changes they made without providing reason because they don’t realize that their description text is simply duplicate of the “files changed” section of the PR. It’s similar to those problematic code comments that say nothing but describe what the code does — it’s better to not include those and let the code speak for itself.

So all that to say, I interpret a “Description” to be far too specific of a heading that leads the author in the wrong direction of describing the changes they made rather than explaining why they made the changes or providing other information in a free-form style that is most appropriate for their PR.

[Ivorforce]

Ok, I don't have a super strong opinion on the heading (though if Description doesn't work for you, maybe someone else has another idea that addresses both our concerns).
I'm fine with going with Additional Information, possibly for now™.

[Ivorforce]

Uncontentious, and worth a try. Current state is mergable in my book even with my two open discussions (which don't have an obvious follow-up).

[AThousandShips]

I think this looks good and is a good start

[allenwp]

Status: Needs More Reviews

Ivorius: Needs 2 or 3 endorsements

I’m by no means in a rush to get this merged or reluctant to have more people look at a PR before it’s merged… but I can’t help but comment that it is a bit silly that a PR like this requires more scrutiny, review, and attention than most other PRs that directly affect the behaviour of all games and apps made with the Godot engine. To be honest, the real point to be made here is that maybe we should scrutinize and have more people looking at PRs that are a part of the Godot engine code to match this level of care, but we all know that’s, unfortunately, unrealistic.

[Ivorforce]

Status: Needs More Reviews

Ivorius: Needs 2 or 3 endorsements

I’m by no means in a rush to get this merged or reluctant to have more people look at a PR before it’s merged… but I can’t help but comment that it is a bit silly that a PR like this requires more scrutiny, review, and attention than most other PRs that directly affect the behaviour of all games and apps made with the Godot engine. To be honest, the real point to be made here is that maybe we should scrutinize and have more people looking at PRs that are a part of the Godot engine code to match this level of care, but we all know that’s, unfortunately, unrealistic.

It's a workflow change that affects all contributors, and maintainers especially since they work with the codebase every day. I'd have expected more people to have an opinion, and that it wouldn't be difficult to get a few more people to share their thoughts, but apparently not so :D

Let's wait for until end of week; if nobody else chimes in I think we can just go ahead with this.

[allenwp]

Sounds good. Having a timeframe instead of a required number of approvals is a more reasonable approach for this sort of a PR.

Also, the Godot policy on GitHub is to discourage people from posting comments that say they agree with a PR because it adds nothing to the conversation. Instead, they are encouraged to give a 👍. This means that when there is silence and 👍, there have actually been as many opinions voiced as there are 👍. So I’d say there have been quite a few thoughts shared in this case. (14 currently)

But, yes, very few disagreements with the sentiment and implementation details, likely because most of those were voiced in the previous PR that this one supersedes.

[Akosmo]

I like the "- Closes" line, but I also wonder if people will just link an Issue to close, and that Issue has barely any information and/or barely any (if at all) discussion.
Maybe I'm just misunderstanding how this will be formatted, but I think it'd probably be better if "What problem(s) does this PR solve?" and "Issues/proposals closed by this PR" were made more distinct, if possible...

(making a general comment because I'm getting an internal error and can't comment on the line)

[Calinou]

Looks good to me, aside of stylistic changes.

[Calinou]

It seems I can't post review comments since today (GitHub issue?). What I wanted to point out in #118855 (review) is that we should use ### for headings instead of ## to be consistent with issues created with the bug report template.

[Ivorforce]

It seems I can't post review comments since today (GitHub issue?). What I wanted to point out in #118855 (review) is that we should use ### for headings instead of ## to be consistent with issues created with the bug report template.

That was Allen's initial design, but I asked to use ## because (imo) the lines make it easier to read, especially when people add their own subsections.
The issue template is less likely to get custom subsections because it's a more streamlined process.

[allenwp]

I like the "- Closes" line, but I also wonder if people will just link an Issue to close, and that Issue has barely any information and/or barely any (if at all) discussion. Maybe I'm just misunderstanding how this will be formatted, but I think it'd probably be better if "What problem(s) does this PR solve?" and "Issues/proposals closed by this PR" were made more distinct, if possible...

(making a general comment because I'm getting an internal error and can't comment on the line)

I originally had instructions for how to fill this section out that I shared in the “discussion” section of this PR’s description text, but it ended up being too much text for a template like this. I also had more headings in my first draft that this PR supersedes…

So… I’m not sure how to improve this overall in a way that better communicates this. If a PR author feels a linked issue doesn’t fully describe the problem, then I expect they’ll write more here. And if they don’t, I expect they probably wouldn’t write more even if prompted… hmm.

[Akosmo]

I like the "- Closes" line, but I also wonder if people will just link an Issue to close, and that Issue has barely any information and/or barely any (if at all) discussion. Maybe I'm just misunderstanding how this will be formatted, but I think it'd probably be better if "What problem(s) does this PR solve?" and "Issues/proposals closed by this PR" were made more distinct, if possible...
(making a general comment because I'm getting an internal error and can't comment on the line)

I originally had instructions for how to fill this section out that I shared in the “discussion” section of this PR’s description text, but it ended up being too much text for a template like this. I also had more headings in my first draft that this PR supersedes…

So… I’m not sure how to improve this overall in a way that better communicates this. If a PR author feels a linked issue doesn’t fully describe the problem, then I expect they’ll write more here. And if they don’t, I expect they probably wouldn’t write more even if prompted… hmm.

I suppose you're right. We also wouldn't want to add much more, so it doesn't become overwhelming for authors, as you said. Overall I'm happy with the changes and willing to see how this goes! It's definitely in the right direction.

[Akosmo]

Speaking as someone who only started contributing during the development of 4.7, has seen many different PRs at this point, and also frequents meetings.

This looks good to me, and I agree with your points.
I have seen many PRs that barely explain the changes made, if at all. Prompting an explanation at any degree is a great start at least! Otherwise it can be hard to tell what changes were made, which considerations were taken into account, and specially which problems are fixed by the PR. An explanation would help reviews for sure.

Hopefully the AI line has some effect.

[Repiteo]

Thanks!