Create documentation manual PR merge guidelines

[skyace65]

Basically does what the title says. The majority of manual documentation PRs get merged by Mhilbrunner and myself. I wanted to make this guide to more or less explicitly give permission (you don't need it now) to merge PRs along with some guidance on when to do so.

[skyace65]

Fixed.

[Mickeon]

I am okay with the actual rules themselves but my position is biased.

[skyace65]

Did some of Mickeon's fixes. Still figuring out how to simplify the one section.

[Ivorforce]

Generally i think this is a great change of policy that empowers maintainers and spreads the load of the documentation team!

The only thing I don't agree on are the required approval counts.
Your proposal matches requires approval counts to the severity. This makes some intuitive sense - more change, more eyes needed - but i don't think it's the right shape.
I would instead match the requirements we have for engine changes: "at least one approval from each involved team" are required for a merge. For example:

• A large change to the core section requires an approval from core and one from documentation.
• A large change to the script editor requires an approval from gdscript, editor, and documentation
• ... and so on.

Not only would this reframe be easier to understand (since it matches our existing strategy), but require experts to sign off changes to their areas, which a pure count wouldn't handle.

[Ivorforce]

Since this section is aimed at maintainers, and regular contributors won't find it helpful, I'd place it elsewhere. We had other maintainer-aimed sections weaved in with the 'regular' sections before and people got confused.

My proposal would be other/release_management/.

[skyace65]

Will move this after addressing other feedback so reviews don't get marked as outdated automatically.

[Ivorforce]

I think it's fine to merge your own PR when there are sufficient maintainer approvals.

Suggested change

	Like the engine, you should not be doing self-merges unless it's an emergency. As an
	Like the engine, you should not be merging without maintainer approvals unless it's an emergency. As an

[AThousandShips]

"should not merge things without approvals" I'd say

[Ivorforce]

Works for me

[skyace65]

I'm going to disagree. If there are sufficient maintainer approvals, the last person who gave approval should have merged it. If they haven't then I think the PR creator should assume it's being left open for review by others for a reason.

[Ivorforce]

Yes, and the reason is them forgetting to merge it most of the time. I've seen it often enough.
We could alternatively say you have to poke the reviewer to ask instead, but I think it's worth trying the 'liberal' option before we reject it.

[skyace65]

That generally hasn't been my experience with the docs, but that's also a matter of me and mhilbrunner do the majority of merges. I'll defer to his opinion whenever he reviews this PR.

[Ivorforce]

Suggested change

	For PRs like these you can merge them with only your own approval. The following
	For PRs like these you can merge them with only one maintainer approval. The following

[skyace65]

I'm not following with how this phrasing is better. If no one has reviewed a PR yet then your approval would be the only one. If someone else already reviewed something like this, you would give your own approval before merging, in which case it would be two maintainers (including yourself)

[Ivorforce]

This one hinges on my earlier change made earlier, where a maintainer might approve a pr and forget to merge it. In that case, someone else can merge it based on the approval (e.g. PR opener, or another stray maintainer).
If you reject the earlier edit, this one doesn't work as well.

[skyace65]

@Ivorforce So here's the issues I see with what you're proposing.

One, the large review requirement is due to the nature of documentation writing. We want as many eyes on large changes as possible to find as many possible grammar issues, or even just to double check phrasing and make sure everything is understandable. This PR itself is only one page, and has had 3 reviewers before you each find a substantial amount of grammar and spelling issues.

Two, a team review requirement is largely unnecessary in most cases. If someone is reviewing a documentation PR they're generally looking at it from two angles, spelling/formatting/grammar kind of stuff, and fact checking. Broadly speaking most documentation doesn't require the matching team to fact check it. As an example, when the shader code preview was added to the shader editor and had to be mentioned in the documentation, you don't need the shader team to check it over, anyone could download the latest beta, check how it works themself and say "yup, the documentation description matches what it does", or even go to the PR that added the feature and check the description (assuming it's detailed enough). I don't want PRs being held up by team reviews that aren't necessary. And yes there are plenty of times where they are necessary, as an example Mihe was crucial for the Jolt physics documentation to make sure my understanding and phrasing was technically correct, but those PRs generally aren't common.

[Ivorforce]

@Ivorforce So here's the issues I see with what you're proposing.

One, the large review requirement is due to the nature of documentation writing. We want as many eyes on large changes as possible to find as many possible grammar issues, or even just to double check phrasing and make sure everything is understandable. This PR itself is only one page, and has had 3 reviewers before you each find a substantial amount of grammar and spelling issues.

The same case could be made for large PRs.
Practically we will still request more reviewers for large PRs, and I concede we could formalize this in the text too. The same is true for the engine after all. But I wouldn't make it the primary driver; that's the "one per area" review requirement, matching upstream.

Two, a team review requirement is largely unnecessary in most cases. If someone is reviewing a documentation PR they're generally looking at it from two angles, spelling/formatting/grammar kind of stuff, and fact checking. Broadly speaking most documentation doesn't require the matching team to fact check it. As an example, when the shader code preview was added to the shader editor and had to be mentioned in the documentation, you don't need the shader team to check it over, anyone could download the latest beta, check how it works themself and say "yup, the documentation description matches what it does", or even go to the PR that added the feature and check the description (assuming it's detailed enough). I don't want PRs being held up by team reviews that aren't necessary. And yes there are plenty of times where they are necessary, as an example Mihe was crucial for the Jolt physics documentation to make sure my understanding and phrasing was technically correct, but those PRs generally aren't common.

This is untrue for core and gdextension documentation changes (which are the only areas I can judge). All the substantial PRs I saw there were difficult to fact check and did require expert knowledge. Fact checking requires somebody who understands the code, and a documentation error would be as catastrophic as a bug in the code.

I agree that an expert isn't needed in all kinds of PRs, but they're definitely needed for the big ones to look over the facts. It doesn't need to be a thorough review either; just them reading it once and approving would be fine. But I would not want to merge a PR detailing and documenting an important engine feature without the expert having read it at least once...

[Ivorforce]

Mandatory expert involvement has the added benefit that they're aware of the documentation and know how and when to change it, taking some ownership.
Isn't more maintainer involvement in the docs, and a relief of the docs team, one of the major goals of this change? :)

[skyace65]

@Ivorforce the purpose of this PR is to formalize the way I more or less handle things now as a guideline, and give others explicit permission to merge since that's not stated anywhere now. And also once we have these guidelines merged, add on rules for specific scenarios that are followed but not written down. (main one being the demo project PR requirement for code changes to the first 3D and 2D games).

There's very few parts of the documentation that I haven't touched, and in my experience the vast majority of doc PRs (And I'm talking about just the non-objective fix ones) do not require teams to be involved for fact checking.

You are asking us to require reviews from teams to change documentation, but not actually requiring those teams to give reviews. I have seen a PR sit for months with no review from a team when it was specifically requested. If you want teams to feel a sense of ownership, those teams need to proactively help us out and get involved. Several already have, the XR team has been great and responsive at reviews, as has the physics team when it comes to jolt. But I am not going to cede ownership of documentation to the teams with no requirement on their end to do anything.

[Ivorforce]

Alright. I can't say I'm fully convinced, but that's not important.
Thanks for considering my viewpoint. If the policy as written is the one you prefer, I'm happy to support it :)

[AThousandShips]

Looks good generally, just needs some of the suggestions applied

[Mickeon]

I had approved myself in principle already. No need to wait much more on this.

[skyace65]

Will try to address the last few things tonight time permitting. If not tonight then Sunday, will be busy all day tomorrow.

[skyace65]

Was able to update the simple stuff tonight but I had less time than I thought to work on this. Will continue reviewing the rest tomorrow. Also definitely want a review by mhilbrunner before a merge.

[Calinou]

Looks good to me.

[skyace65]

I'm done updating for reviews for now. Main thing at this point is I need mhilbrunners review. Once he's given this a review and anything from that has been addressed I'll move the page location as well. Also waiting for his opinion on the self-merge rule before making any changes.