I don't think "stands for" is the right wording. That makes it sound like 'nits' is an acronym, which it isn't.

Thank you for pointing this out. I'm not a native speaker so this is really helpful. Does "means" sounds better?

Yes, that would sound better.

I'm not sure the description is quite accurate. IMHO nits are just minor issues, whether the changes needed to address the minor issue are large or small is irrelevant. For example, comment typos for a PR that changes code would probably be considered a nit especially because comments don't affect the code, but it could be there are a lot of typos to fix or just one.

Hmm.. I copied this phrase from the onboarding guide, maybe that one needs to be more accurate too?

How does "request for changes that are not essential" sound?

That sounds better I think.

It might be worth also noting Github's PR review mechanism too if we want to advocate its usage.

Does adding a link to the video tutorial(https://www.youtube.com/watch?v=HW0RPaJqm4g) and the documentation(https://help.github.com/articles/reviewing-changes-in-pull-requests/) suffice?

That might be a good start.

I think we may want to add some text about the validity of approvals (either via 'LGTM' or Github's PR review) after changes were made since the approval. For example, when someone approves a PR using Github's mechanism, it will still show "Approved" even after someone pushes more changes after that approval is made, which can be misleading (at least at first glance).

Notes added. Thanks for the suggestion!

The indentation is off here and possibly other places below.

Addressed. Thanks for pointing this out.

This isn't actually true, the commit history of your PR is available until you delete your fork branch, at which point it disappears.

Thank you for pointing this out. Maybe "The commit history of your Pull Request, however, will stay intact on the Pull Request page(as long as you don't delete your fork branch, at which point it disappears)." is more accurate?

The commits won't necessarily be squashed into one commit per PR, they'll be squashed into one commit per logical change (it's hard to specify what a logical change is).

Hmmm..maybe suggesting the new contributors checkout the previous commits to have an idea about the size of a logical change? Or a link to a specific example(preferably one with lib, src, doc, test changes but is still one logical change)?

I think this one could be a good example. #2921 Although it's two logical changes.

Ooops, strike that, that one landed as two commits too. I will try to dig up a better example(any help would be appreciated!).

OK, I think this commit 0b5191f could be a good example. There are a lot of lines changed, but it's still one logical change.

links to the pull request and relevant issues

Thanks, I will add this to the sentence.

Collaborator? Also, collaborators can use GitHub's Approve button.

Do you mean this should be singular? I will add a phrase mentioning the Approve button, thanks

My only concern with this is the addon tests are actually generated by the documentation. I do recall a time where our test suite was broken due to addon docs being changed and merged without the CI being run. Maybe worth mentioning?

TBH I feel that it's worth running the CI even on doc changes, otherwise there's no record that the linter was run. I know that collaborators are supposed to run make lint as part of the landing process, but that inevitably doesn't always happen, and it has caused issues in the past.

Oh, I didn't know about this! Thanks for the explanation, maybe I should also remind new contributors about this in the Step 5: Test section? So is it like:

• If you touch the code snippets inside addons.md, you need to run make -j8 test
• If you touch the code snippets inside any documentation, you need to run make lint

Is there anything else that can be added to this list?

make -j4 test does run the addons tests, and we currently tell everyone to run that on their PR. I'd prefer just leaving it like that, we don't want to be telling people that they don't need to run the full test suite (they'll work that out for themselves).

So the instructions here should be something like "as long as you touch the code snippets inside the documentation, you need to run make test"? Or just leave out the documentation part and state that every PR, including doc PR, needs to pass tests?

@joyeecheung So currently in the guide it says that:

Make sure the linter is happy and that all tests pass. Please, do not submit patches that fail either check.

I think that this sentence is sufficient, it says that you should always make sure the tests and linter pass.

So for this part that we are commenting on, I will just change this back to "..and pass a CI test run". Maybe I can add some notes at the end to remind people of the needs of running tests for changes to doc? Just feeling odd to let this practice go undocumented.

Well, what is currently documented is that you should always run make -j4 test no matter what, which is strictly correct. What isn't documented is that in practice you can sometimes get away with just running the linter (or the specific test you changed). That's not something we really want to encourage, especially as it's a tricky issue (some doc changes do need make test, some test changes do need make test). Documenting something is effectively legitimising it, which I (personally) don't think we want to do.

Ah, I see your point, but what I meant was to stress that doc PR needs to pass the tests too (and possibly explain why , or not if that expose too much details). I didn't mean to remind people that there is a way to skip the tests. Because I think the previous section is not very clear about doc PR needs tests too. After all it could be unexpected to new comers that you need to set up the whole compiler tool chain to get some markdown changes pass the review. But they do need to, so a heads up could be nice.

That said, since in practice this could be flexible, I think I can leave this as-is. Thank you for the clarification!

make sure that it

Fixed. Thanks for spotting this one :)

Microscopic nit: There is a trailing space at the end of this line.

Should have trimmed the spaces :P. Addressed. Thanks!