Create documentation manual PR merge guidelines#91
Conversation
skyace65
added
the
content:new page
| - Removing a dead link that doesn't need to be replaced. For example, community | ||
| tutorials that have been removed by the author. Links that do need to be | ||
| replaced are any the text relies on. For example "Download Tool ABC Here and then |
There was a problem hiding this comment.
| - Removing a dead link that doesn't need to be replaced. For example, community | |
| tutorials that have been removed by the author. Links that do need to be | |
| replaced are any the text relies on. For example "Download Tool ABC Here and then | |
| - Removing a dead link that doesn't need to be replaced. For example, a link to a removed community | |
| tutorial. Links need to be | |
| replaced if any text relies on them. For example, "Download Tool ABC here and then |
Maybe consider splitting Line 41-45 into two bullet points?
There was a problem hiding this comment.
I'll do the rewording but that can't be split into two, each bullet point is an example of an objective change, having an explanation of what isn't as a bullet point doesn't work.
There was a problem hiding this comment.
I was thinking about splitting it into "replacing" and "removing", and then putting "replacing" first. But I see why you would want to keep them together, if you think it flows better that way.
| - Removing a dead link that doesn't need to be replaced. For example, community | |
| tutorials that have been removed by the author. Links that do need to be | |
| replaced are any the text relies on. For example "Download Tool ABC Here and then | |
| - Replacing an outdated link if any text relies on it. For example, | |
| "Download Tool ABC here and then do this with it." If the workflow is different, more | |
| substantial changes need to be made. | |
| - Removing a dead link that doesn't need to be replaced. For example, a link to a | |
| removed community tutorial. |
| There are two important things to keep in mind. First, updating a page for engine | ||
| changes is **not** automatically an objective fix in this context. If there's a | ||
| substantial ammount of text that's changed that would be considered a content | ||
| change. |
There was a problem hiding this comment.
| There are two important things to keep in mind. First, updating a page for engine | |
| changes is **not** automatically an objective fix in this context. If there's a | |
| substantial ammount of text that's changed that would be considered a content | |
| change. | |
| There are two important things to keep in mind. First, updating a page for engine | |
| changes is **not** automatically an objective fix in this context. If there's a | |
| substantial amount of text that's changed, that would be considered a content | |
| change. |
Maybe the second important thing is missing here?
There was a problem hiding this comment.
The next paragraph is the second thing, I'll add a "second" to make clearer.
skyace65
force-pushed
the
Merge-Guidelines
branch
from
71bff7b to
4fbb0c5
Compare
skyace65
force-pushed
the
Merge-Guidelines
branch
from
4fbb0c5 to
9503e75
Compare
|
Fixed. |
Mickeon
left a comment
Mickeon
left a comment
There was a problem hiding this comment.
I am okay with the actual rules themselves but my position is biased.
| - Removing a duplicated word. For example, a page says the following: "this and and | ||
| that node." | ||
| - Fixing a typo. |
There was a problem hiding this comment.
I feel like these two (and a few more) could be generalized as... Something something... punctuation, orthography, spelling...
"Orthographic errors"?
There was a problem hiding this comment.
I might just be dumb, but I had to google what orthographic means so I'm not sure if that's a good term to go with.
Maybe "Fixing spelling, grammar and misplaced words"?
There was a problem hiding this comment.
Well, if you'd have to "list" all three in one line, I suppose it's better as is now to separate them
skyace65
force-pushed
the
Merge-Guidelines
branch
from
9503e75 to
b092e50
Compare
|
Did some of Mickeon's fixes. Still figuring out how to simplify the one section. |
Ivorforce
left a comment
Ivorforce
left a comment
There was a problem hiding this comment.
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
coresection requires an approval fromcoreand one fromdocumentation. - A large change to the script editor requires an approval from
gdscript,editor, anddocumentation - ... 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.
|
|
||
| contributing_to_the_manual | ||
| building_the_manual | ||
| manual_merge_guidelines |
There was a problem hiding this comment.
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/.
|
|
||
| Manual merge guidelines | ||
| ======================= | ||
|
|
There was a problem hiding this comment.
This needs a disclaimer aimed at contributors like https://contributing.godotengine.org/en/latest/other/release_management/merge_guidelines.html:
| .. note:: | |
| This section is targeted at maintainers. Regular contributors to Godot | |
| cannot merge pull requests. | |
| your judgement. This page exists to help guide your thought process on whether or | ||
| not something should be merged. | ||
|
|
||
| Like the engine, you should not be doing self-merges unless it's an emergency. As an |
There was a problem hiding this comment.
I think it's fine to merge your own PR when there are sufficient maintainer approvals.
| 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 |
There was a problem hiding this comment.
"should not merge things without approvals" I'd say
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| example, another maintainer's account was hacked and they merged a PR that just adds | ||
| a bunch of slurs to pages. |
There was a problem hiding this comment.
I would go a little bit easier on the example, since it's only an aside
| example, another maintainer's account was hacked and they merged a PR that just adds | |
| a bunch of slurs to pages. | |
| example, when catastrophically bad content such as advertisements or slurs were accidentally merged. |
There was a problem hiding this comment.
Perhaps mention vandalism, spam, or both as well
| If a specific person was requested to review a PR by the creator of the PR, wait a | ||
| few weeks to give them time, even if it has a lot of existing approvals (roughly 4 | ||
| or 5). This isn't a hard rule, if it's been over a month use your best judgement | ||
| depending on the situation. |
There was a problem hiding this comment.
Might as well mention both situations directly, since they closely relate
| If a specific person was requested to review a PR by the creator of the PR, wait a | |
| few weeks to give them time, even if it has a lot of existing approvals (roughly 4 | |
| or 5). This isn't a hard rule, if it's been over a month use your best judgement | |
| depending on the situation. | |
| Even when there are enough approvals, give other maintainers some time to add their review (a few days is usually fine). | |
| This especially applies if a specific person's review was requested. If more than a month or so passes without that review coming in, use your judgement to decide whether to poke the person or skip that review. |
|
|
||
| Objective fixes are, as the name implies, any change where the PR is an unarguable | ||
| fix for a problem with the documentation, where the type of fix can't be debated. | ||
| For PRs like these you can merge them with only your own approval. The following |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| Broadly speaking all PRs can be put into three categories: **objective fixes**, | ||
| **content changes**, and **backend changes**. | ||
|
|
||
| Objective fixes |
There was a problem hiding this comment.
I think adding 'trivial' could better inform some of your later caveats.
| Objective fixes | |
| Trivially correct fixes |
There was a problem hiding this comment.
I'm not sure I understand, could you elaborate more please?
There was a problem hiding this comment.
There are many fixes that are 'objective' but complex enough to require a second set of eyes.
The true differentiator of a PR being mergable without many eyes is that they're trivially correct. That means it's obvious that they're correct and that they don't need more eyes, such as a spelling correction.
This prefaces your later caveats, such as the section on engine changes:
There are two important things to keep in mind. First, updating a page for engine
changes is **not** automatically an objective fix in this context. If there's a
substantial amount of text that's changed, that would be considered a content
This could be considered to be an objectively correct fix (updating a wrong state into a right one), but it's not a trivial one, so it needs some eyes on it (ideally documentation) to make sure it matches in narrative, style, spelling, etc.
There was a problem hiding this comment.
I guess you can argue that, while the listed fixes are "objective" at a glance, that descriptor could be pushed to its limits in practice.
For example, and I do recall similar cases, "the nodes parent" could be corrected as both "the nodes' parent" and "the node's parent". Other times, a verb is completely missing in the middle of a sentence. It becomes harder to make a purely objective assessment.
| --------------- | ||
|
|
||
| Content changes are any change that change where the PR is not considered an | ||
| objective fix. This includes new pages, updated paragraphs, reworded sections, and |
There was a problem hiding this comment.
| objective fix. This includes new pages, updated paragraphs, reworded sections, and | |
| trivial fix. This includes new pages, updated paragraphs, reworded sections, and |
|
@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. |
The same case could be made for large PRs.
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... |
|
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. |
|
@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. |
|
Alright. I can't say I'm fully convinced, but that's not important. |
| typically wait at least two weeks, and it should have at least 3 PR approvals | ||
| including your own. | ||
|
|
||
| This is also the one situation where approval from the documentation team is |
There was a problem hiding this comment.
| This is also the one situation where approval from the documentation team is | |
| This is also the one situation where approval from the documentation team is |
5 participants
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.