Content: README - add guidance on how to make changes to PR source

[hamishwillee]

It is usually easiest if the reviewer has the option to edit the source of a submitted PR directly rather than have to ask the original editor to make changes. It might be useful to add guidance on how to do this in the README.

E.g. how to get someone else's PR source locally, make changes and push them back. Sometimes forked repos do not allow this, so perhaps also explanation of what the forked settings must be to allow this.

[sideshowbarker]

It is usually easiest if the reviewer has the option to edit the source of a submitted PR directly rather than have to ask the original editor to make changes. It might be useful to add guidance on how to do this in the README.

Since GitHub has good docs on that, maybe the guidance we add to our README could amount to just being some minimal text that links to the relevant existing GitHub guidance:

• https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork
• https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

[vkWeb]

@hamishwillee I agree with you; it'll accelerate our review process. Asking PR authors to fix small things would be too time consuming (and trivial), we members with push access should fix those small things ourselves.

@sideshowbarker Yes - GitHub has good docs on achieving that but it is overkill. Cloning fork, pulling branches, then deleting all of those afterwards is just too much work. We can achieve that more easily by following: https://gist.github.com/vkWeb/dcec82b079f1edc19478ddb58b0ffc5e; this has been battle-tested in my past experience with freeCodeCamp.

Note:

• only members with push access can push commits to PR branches
• during PR creation the author must enable "allow edits from maintainer" checkbox

If the above two conditions are fulfilled then there will be no problem dealing with PRs.

cc @chrisdavidmills

[sideshowbarker]

Cloning fork, pulling branches, then deleting all of those afterwards is just too much work.

You’re right — but I think we’re fortunate these days to have tooling around to help automate the steps

We can achieve that more easily by following: https://gist.github.com/vkWeb/dcec82b079f1edc19478ddb58b0ffc5e

That looks great. And looking at it makes me recall the official GitHub command-line client has a command for automating it:

gh pr checkout {<number> | <url> | <branch>} [flags]

https://cli.github.com/manual/gh_pr_checkout

…and so does the older hub command:

hub checkout PULLREQ-URL [BRANCH]

https://hub.github.com/hub-checkout.1.html

So maybe in our docs we should link to one or both of those.

(I don’t use the gh client at all myself, mostly because it seems really slow in my environment at least. I instead use hub for a lot of things — though not for checking out PR branches, because I wrote https://github.com/sideshowbarker/gpr for doing it.)

[Ryuno-Ki]

I'm wondering, why minimal changes couldn't be done from within the Web UI?

[sideshowbarker]

I'm wondering, why minimal changes couldn't be done from within the Web UI?

Yeah, that’s a good point — I think in practice we’ll find that many changes could be reviewed from the Web UI without ever needing to do anything from the command line or with some external git GUI client. Or maybe most changes. And not just minimal changes.

Going to the command line or external git GUI client is useful or necessary when reviewing patches to runnable code, when you need to test the patch as part of the review. But for this repo, since we’ll not be reviewing runnable code, it won’t be necessary to test the patches.

That said though, I think there’s a plan to have a linter here (almost certainly written in JavaScript, and running under NodeJS) that reviewers and contributors can optionally run themselves from the command line to lint-check their patches. But even in that case, I think we’ll have a CI check set up which runs that linter automatically (and blocks the patch from being merged until there are no lint errors) — so reviewers/contributors won’t necessarily even need to run the linter themselves.

[Ryuno-Ki]

What other repos could be learned from?

[peterbe]

In general I think Yari needs to be careful to not try to become a leaky abstraction for good old solid git. And the README mustn't try to have everything that Git's and GItHub's own documentation does.
But I definitely think it's worth keeping an eye on things that are repeating itself over and over and that's hurting you.

Personally, I wrote a Python script so I can just type gg branches someusername:branchname and it takes care of fetching and pulling etc. (I built this before GitHub's own CLI and now it's muscle memory). But it has saved me sooo much time over the years.

So please keep an eye out for things that we can abstract with tooling if we start to see an annoying pattern that could be improved for your super-contributors.
Also, I like the idea of a little blurb about linking to the GitHub CLI documentation.

[vkWeb]

I'm wondering, why minimal changes couldn't be done from within the Web UI?

@Ryuno-Ki This is actually a very solid idea (for content-related reviews)! :rocket:
I didn't know we can do changes to PRs via GitHub's UI :sweat_smile:

[hamishwillee]

FWIW IMO the best docs sites have two links on every page

• Edit on github - opens page on github for editing (or to create account/login)
• Bug report - raises a bug report seeding the report with a link to the associated page (ie a bug template). This can be done with a little bit of playing with URL params.

"Generally" the problem with web UI is that it is only useful for fixes to existing pages - i.e. minor typos. As soon as you need to add new pages or images the UI is rubbish (deletion is fine).

I think MDN may have even bigger problems because we don't support markdown. I.e. it may be quite easy to "break the build" unless we test using the proper toolchain. Could be wrong - haven't tried a local build yet.

[chrisdavidmills]

Hi folks!

I agree that it is good to consider making your own changes to a pull request if the change is really simple, or if the submitter is taking too long perhaps. I've merged the reviewing guide: https://github.com/mdn/content/blob/main/REVIEWING.md

In it, I've included some details of how to make some changes to a submitted PR, and included some of the links you suggested, including @vkWeb 's nice little concise guide.

[Ryuno-Ki]

I just noticed https://github.com/mdn/content/issues/550#issuecomment-749864664