Reviewers guide

[davcri]

We should agree on some guidelines in order to have consistent merging/closing process for the huge number of pull requests that are arriving.

I'll gathered some tips found in the gitter channel fcc/contributors.

UPDATE: please refer to this PR where we can add commit to update this reviewers' guide!

---

OLD message content:
I know that Github-issues are not for this, but it's a temporary solution while we found a better place.

Please comment and discuss here and I'll update this guidelines.

General tips

Quincy Larson on Gitter said:

"Note that these pull requests do not have to be perfect. It is better to have an incomplete article - or one with imperfect English - than to not have that article at all. As long as a pull requests improves or expands the guide in some meaningful way, we can accept it. Future contributors will come through and smooth out the rough edges in these stories."

See also these links:

• Article style guide
• Moderator guidelines: this one is not specific to this project, but some of the QA section has relevant tips.

PR Review Ordering

Start from older pull requests.

You can use this filter to list relevant pull requests:
is:pr is:open sort:updated-asc -label:platform -label:enhancement -label:invalid -label:"changes requested"

Accepting PR

Squash commits

Make sure to use the Squash and Merge option when you merge the PR.
This will keep the commit history clean.

Use meaningful titles

Under discussion, see here: #1853

Closing PR

When to close a PR

A PR should be closed if:

• There is zero/little effort in it (e.g: copy pasting from another source like Wikipedia)
• There is copied text from a copyrighted source (see also Add citation suggestion to README.md #2503)
• Does not respect the Article style guide
• It is stale (if a change is requested and there weren't any activity for about 2 weeks)

Remember that a PR can always be reopened; merge can be reverted.

Template text for closing PR

Quincy Larson suggested this text:

Thank you for your pull request. Please read this style guide: https://github.com/freeCodeCamp/guides#article-style-guide

I am closing this pull request for now. Please let me know if you have any
questions.

Requesting changes

If the pull request is not perfect we can:

• fix minor issues by ourselves
• request changes to the contributor

Adding Labels

• content is for pull requests that modify the content of articles on the guide (e.g.: new articles or updating articles)
• duplicate is for pull requests that have the same content
• change requested is for pull requests that need a change before getting merged.
• stale: is for pull request with "change requested" label that doesn't get activity after about 2 weeks. A stale pull request should be closed (example: Added Bootstrap JS and jQuery Scripts, and added Bootstrap Modal article #235)

[johnkennedy9147]

Once agreed we could make a reviewers guide doc and put it in the TLD and make reading it a prerequisite to being granted write access.

[johnkennedy9147]

@systimotic @Bouncey @QuincyLarson @HKuz @texas2010 @dhcodes your input would be good here, especially for newbies like myself

[johnkennedy9147]

maybe include a section on useful filters etc that make life easier
for example the below clears out a lot of PRs that I wouldnt be dealing with and sorts oldest first
is:pr is:open sort:updated-asc -label:platform -label:enhancement -label:invalid -label:"changes requested"

[zuzana-kunckova]

@davcri , great overview, thank you!

Re plagiarism. -I use this checker to evaluate the extent to which someone has copy pasted from another source.

[davcri]

Nice site @zk433 ;) At the moment I just copy some paragraph from the article and search for it through DuckDuckGo (or other search engines).

[QuincyLarson]

@davcri This is great! We should add this to the Readme under a heading "How We Review Pull Requests"

One note - instead of differentiating between copyrighted and creative commons text, we should just say "If you quote text - or even paraphrase it - you should cite the source."

We can set up a CI task to check for plagiarism, but for now, I think @zk433's tool is a great resource for manually checking suspicious passages.

[erictleung]

At least in the short term, while we deal with the near 1000 PRs (!!!), we should give some guidelines on how to deal with these duplicates.

Today, I went looked through PRs that were least recently updated (a built-in sort in GitHub) and then started there. With that PR, I searched through all the PRs for any other issues with similar content (e.g. today I found like 3 PRs modifying the same cryptography article). I skimmed through each of them to see how much content they added and started merging from the oldest to the newest.

This will most likely induce merge conflicts. If the newest PR doesn't contribute much more, I'll close it. If it does add some information, I'll try resolving the conflict myself through GitHub.

This process is slow, I have to admit. But I think it is most thorough as you're bound to run into lots of merge conflicts as we start merging through all the PRs. Just a thought.

[davcri]

I prepared a PR to improve the README: #2976. It's mostly taken from the first post of this issue. Feel free to share your thoughts! The more the comments, the best the guidelines will be.

[QuincyLarson]

@erictleung Yes - I agree that this is the best process for dealing with potential conflicts. The good news is that with articles, conflicts are usually pretty easy to address right in GitHub's editor.

Once we work through this backlog we should have significantly fewer conflicts.

[HKuz]

@davcri - this looks excellent, thanks for compiling all this information in one place!

I agree with @erictleung 💯 on trying to merge what we can (and resolve conflicts, if necessary).

Here are a few comments that have come up on other issues or in recent conversation:

If a PR breaks the build (the Travis CI check fails ❌ ) there are two likely sources (which need to be addressed/fixed before the PR can be merged):

1. The contributor created a new folder or subfolder for their article, but didn't include an index.md file - every folder in src/pages needs an index.md file in it. This has happened when the contributor creates both a new folder, then a subfolder, writes an article in the subfolder but forgets to put a stub index.md file in the new folder
2. The article doesn't have a title field at the top - every article needs this as the first 3 lines of the index.md file:

---
title: Title of the article that shows up in the site's menu
---

Just learned this one from reviewing #4932 - if a PR adds text (like only a couple links in the More Information section), but does NOT remove the stub language, the Normalise.js script will revert that file back to a stub (comment from @Bouncey over there):

Normalise looks for 'This is a stub... in the file via a RegEx. If found, the entire file is re-stubbed. This behaviour was intended, allowing us to update all stubs if the template stub changed for any reason.

Knowing this, if a PR only adds links to a stub, we should ask the contributor to add a short description to start the article and remove the stub language, otherwise their contribution will be erased when the next Normalise commit is merged.

And finally (not critical), when you squash & merge, you won't automatically get GitHub activity "contribution" credit. If you want it (and reviewing a PR certainly deserves it!!), make sure to "approve" the PR (over in the "Files changed" tab) before you merge it 😄

[HKuz]

Hi everyone - I'm going to close this issue now that @davcri 's PR #2976 was merged - it created a new CONTRIBUTING.md file with these suggestions incorporated. Feel free to re-open if you want to continue the discussion, open a new issue, or submit a PR against that file 👍