Skip to content
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.

Sign up for GitHub

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

doc: adding "rules of thumb" #12744

Closed
wants to merge 9 commits into from
Closed

doc: adding "rules of thumb" #12744

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9a79fe6
doc: add "rules of thumb" section
refack Apr 29, 2017
953c7e0
[squash] add two missing '.'s
refack Apr 29, 2017
9ab59c1
[squash] add some line breaks, and fix a few typos
refack Apr 29, 2017
1691471
[squash] trying to form full sentences, and adding a line in the "com…
refack Apr 29, 2017
edc0b43
[squash] more explicit working or style and linters
refack Apr 29, 2017
011263e
[squash] Collapse explanations
refack Apr 29, 2017
3a9ee5b
Removing personal pronouns
refack Apr 29, 2017
fccb06a
[squash] its -> it is
refack Apr 29, 2017
3fd0eb2
[squash] typos & remove "mark as blocked"
refack May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,34 @@ works.

This document will guide you through the contribution process.

### Rules of thumb

1. #### Provide motivation for the change
Why will this change make the code better; does it fix a bug, is it a new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to use full sentences here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, PTAL

feature, etc. This should be in the commit messages as well as in the PR
description.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this should go into the section describing commit messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean having it there as well?
Since this is a general section, IMHO people should have this is mind when thinking about a contribution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don’t think I’ve ever seen somebody make a PR without having a reason to do so 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I just like to churn 😄, reindent, reorder lines, rethink names etc... But I keep it to my code.
I ment the contributor should know they will need to explicitly provide their motivation.
P.S. adding to section about commit message

2. #### Don't make _unnecessary_ code changes
Things that are changed because of personal preference or style, like:
renaming of variables or functions, adding or removing white spaces,
reordering lines or whole code blocks. These sort of changes should have
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for using full sentences, that will at least help everyone who’s not a native English speaker

a good reason since they cause unnecessary ["code churn"](https://blog.gitprime.com/why-code-churn-matters).
As part of the project's strategy we maintain multiple release lines, code
churn might hinder back-porting changes to other lines. Also when you
change a line, your name will come up in `git blame` and might hide the
previous writer of the code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn’t really sound like a “rule of thumb” to me – It doesn’t really tell contributors anything about what they should or should not do, does it? (Also: “churn” might be a bit too much of a jargon-y term, I could imagine a lot of people don’t know what that means)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"churn" is also quite subjective. At the code and learn events, we intentionally encourage participants to focus on very small changes that end up leading to quite a bit of churn individually. We have also become much better at very quickly reviewing and landing smaller changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was changes that are just personal style changes (or a result of some automatic tool).
I'll make it more explicit, to things like

  • renaming variables
  • adding/removing white spaces
  • reordering lines
    Anything else?

3. #### Keep your change-set self contained but at a reasonable size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: self-contained

Use your good judgment when making a big change. If you can't think of a
good reason but need to make a very big PR, try to break it into smaller
pieces (still as self-contained as possible), and cross-reference them.
You can also mark some of them as `blocked` pending the others.
4. #### Be aware of our style rules
As part of acceptance of a PR the changes must pass our linters. For C++ we
use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: adjustments

should keep you in line.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“keep you in line”?

For JS we use this [ruleset](https://github.com/nodejs/node/blob/master/.eslintrc.yaml)
for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules)
For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md)

### Step 1: Fork

Fork the project [on GitHub](https://github.com/nodejs/node) and check out your
Expand Down