-
-
Notifications
You must be signed in to change notification settings - Fork 26
Create documentation manual PR merge guidelines #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,110 @@ | ||||||||||||||||||
| :allow_comments: False | ||||||||||||||||||
|
|
||||||||||||||||||
| .. _doc_manual_merge_guidelines: | ||||||||||||||||||
|
|
||||||||||||||||||
| Manual merge guidelines | ||||||||||||||||||
| ======================= | ||||||||||||||||||
|
|
||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a disclaimer aimed at contributors like https://contributing.godotengine.org/en/latest/other/release_management/merge_guidelines.html:
Suggested change
|
||||||||||||||||||
| There are no rules on which maintainers are or are not allowed to merge changes to | ||||||||||||||||||
| the manual. You do not need to be part of the :team:`Documentation` to do so. | ||||||||||||||||||
|
|
||||||||||||||||||
| The following are guidelines on when a PR can be merged. These are not hard rules | ||||||||||||||||||
| that you must adhere to. If you're a maintainer then that means we already trust | ||||||||||||||||||
| 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 | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to merge your own PR when there are sufficient maintainer approvals.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "should not merge things without approvals" I'd say
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and the reason is them forgetting to merge it most of the time. I've seen it often enough. |
||||||||||||||||||
| example, another maintainer's account was hacked and they merged a PR that just adds | ||||||||||||||||||
| a bunch of slurs to pages. | ||||||||||||||||||
|
Comment on lines
+17
to
+18
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would go a little bit easier on the example, since it's only an aside
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. | ||||||||||||||||||
|
Comment on lines
+20
to
+23
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well mention both situations directly, since they closely relate
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| Broadly speaking all PRs can be put into three categories: **objective fixes**, | ||||||||||||||||||
| **content changes**, and **backend changes**. | ||||||||||||||||||
|
|
||||||||||||||||||
| Objective fixes | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding 'trivial' could better inform some of your later caveats.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand, could you elaborate more please?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||
| --------------- | ||||||||||||||||||
|
|
||||||||||||||||||
| 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 | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||||||||||||||||||
| would constitute an objective fix: | ||||||||||||||||||
|
|
||||||||||||||||||
| - Removing a duplicated word. For example, a page says the following: "this and and | ||||||||||||||||||
| that node." | ||||||||||||||||||
| - Fixing a typo. | ||||||||||||||||||
|
Comment on lines
+36
to
+38
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like these two (and a few more) could be generalized as... Something something... punctuation, orthography, spelling...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, if you'd have to "list" all three in one line, I suppose it's better as is now to separate them |
||||||||||||||||||
| - Fixing grammar. For example, changing "it's" to "its". | ||||||||||||||||||
| - Fixing a link that no longer works because content was moved. | ||||||||||||||||||
| - 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 do this with it." In situations like | ||||||||||||||||||
| that a replacement needs to be found, or if the workflow is different, more | ||||||||||||||||||
| substantial changes need to be done. | ||||||||||||||||||
| - Replacing an image that does not show what is described in the text. | ||||||||||||||||||
| - Replacing an outdated image with no major text changes. Minor text changes like a | ||||||||||||||||||
| rename are fine, for example, changing ``Node`` to ``Node3D``. | ||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
Comment on lines
+50
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Maybe the second important thing is missing here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The next paragraph is the second thing, I'll add a "second" to make clearer. |
||||||||||||||||||
|
|
||||||||||||||||||
| Second, even though these are relatively minor changes, you are still responsible | ||||||||||||||||||
| for ensuring all changes are correct adhere to :ref:`image guidelines <docs_image_guidelines>` | ||||||||||||||||||
| if applicable. | ||||||||||||||||||
|
|
||||||||||||||||||
| Content changes | ||||||||||||||||||
| --------------- | ||||||||||||||||||
|
|
||||||||||||||||||
| 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 | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| added sections. | ||||||||||||||||||
|
|
||||||||||||||||||
| When these PRs should be merged will vary from situation to situation, but you may | ||||||||||||||||||
| use the following sections can be used as a starting point to decide. | ||||||||||||||||||
|
|
||||||||||||||||||
| Small changes | ||||||||||||||||||
| ~~~~~~~~~~~~~ | ||||||||||||||||||
|
|
||||||||||||||||||
| Small edits, such as one paragraph and a screenshot are generally fine with only one | ||||||||||||||||||
| approval. However, you should be leaving the PR open for about a week before merging | ||||||||||||||||||
| in case anyone else wants to chime in and review. | ||||||||||||||||||
|
|
||||||||||||||||||
| Large changes | ||||||||||||||||||
| ~~~~~~~~~~~~~ | ||||||||||||||||||
|
|
||||||||||||||||||
| For larger changes, such as an entirely new page with a substantial amount of | ||||||||||||||||||
| content, or an extensive overhaul of an existing one, you should be waiting longer | ||||||||||||||||||
| and for more approvals. | ||||||||||||||||||
|
|
||||||||||||||||||
| Generally 2 weeks and 3 PR approvals is the minimum for situations like this. | ||||||||||||||||||
|
|
||||||||||||||||||
| Backend changes | ||||||||||||||||||
| --------------- | ||||||||||||||||||
|
|
||||||||||||||||||
| Backend changes are anything that changes dependencies, or the website | ||||||||||||||||||
| itself such as its layout or design. | ||||||||||||||||||
|
|
||||||||||||||||||
| Dependency updates | ||||||||||||||||||
| ~~~~~~~~~~~~~~~~~~ | ||||||||||||||||||
|
|
||||||||||||||||||
| Typically, GitHub's Dependabot will open a PR to update dependencies on GitHub. These | ||||||||||||||||||
| can be merged after you give approval, provided that you've built the branch of that | ||||||||||||||||||
| PR locally and confirmed that nothing has broken. You don't need to check every | ||||||||||||||||||
| page. As an example, if Dependabot submits a PR to update ``sphinx-tabs`` then you | ||||||||||||||||||
| should check about 3 pages that have code tabs of different types and make sure | ||||||||||||||||||
| everything still looks normal. | ||||||||||||||||||
|
|
||||||||||||||||||
| Other changes | ||||||||||||||||||
| ~~~~~~~~~~~~~ | ||||||||||||||||||
|
|
||||||||||||||||||
| For any other change, such as adjusting website style or functionality, you should | ||||||||||||||||||
| 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 | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| required. It doesn't have to be everyone on the team, but it should be approved by 2 | ||||||||||||||||||
| people on the team at minimum. | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/.