doc: adding "rules of thumb" #12744

refack · 2017-04-29T16:26:11Z

Discussion moved to #12791

[edit]

As I understand the term "rules of thumb", they are guidelines one should have in mind and try to adhere to, but are not necessarily hard requirements. Our requirements should be stated in the other parts of the document.

[/edit]

My suggestion for a few "rules of thumb" for code contributions.
These are obviously my personal opinion, so this PR welcomes edit and suggestions. I do think that we should put some loose guidelines in writing.

The following made me think of this, and are not critique!
Ref: #12603 - motivation
Ref: #12735 - size

Open questions:

A significant amount of the first-time contributors don't look at this document at all. So is this the best place to put this?
Are some of these not just rules of thumb but rather requirements that are better moved to appropriate places of the document.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

vsemozhetbyt · 2017-04-29T16:31:45Z

CONTRIBUTING.md

+### Rules of thumb
+
+1. __Provide motivation for the change__  
+   Why will this change make the code better; does it fix a but, is it a new


addaleax · 2017-04-29T16:30:23Z

CONTRIBUTING.md

+   30 files is a reasonable size limit, so unless you have a good reason to make
+   a bigger 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.   


I don’t think we should set up rules like this one, and instead keep trying to use good judgement. I can’t really imagine a “reasonable size” defined for all PRs, it’s really a case-by-case thing. (e.g. #11883 and #11975 are huge PRs and that’s completely reasonable in my eyes).

I'll reword, but IMHO something should be put in writing. For example @vsemozhetbyt made #12735 which is good, but very hard to review ( @vsemozhetbyt ment with love )

addaleax · 2017-04-29T16:30:51Z

CONTRIBUTING.md

+
+1. __Provide motivation for the change__  
+   Why will this change make the code better; does it fix a but, is it a new
+   feature, etc.


This should go into the commit message section, imho.

Ok, good!
Also some PRs don't follow the rules and don't include the original commit messages.

addaleax · 2017-04-29T16:31:36Z

CONTRIBUTING.md

@@ -30,6 +30,22 @@ works.

 This document will guide you through the contribution process.

+### Rules of thumb
+
+1. __Provide motivation for the change__  


No need to make the headings bold, we don’t do that elsewhere either. If you want these to be headings, then go with Markdown’s built-in support for, well, headings ;)

addaleax · 2017-04-29T16:33:46Z

CONTRIBUTING.md

+   As part of the projects strategy we maintain several lines of code, code 
+   churn might hinder back-porting changes to other branches. Also when you 
+   change a line, your name will come up in `git blame` and might hide the
+   previous writer of the code.


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)

"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.

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?

addaleax · 2017-04-29T16:35:25Z

(By the way: I assume it’s by accident, but you keep opening PRs from the nodejs/node repo instead of your fork … we don’t do that except for releases branches, can you try to make sure you open PRs from your fork for the future?)

addaleax · 2017-04-29T16:36:38Z

CONTRIBUTING.md

+   use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide)
+   should 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 rules [custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules)


This basically comes down to “run make test/make lint” before submitting a PR, right?

Be aware (also changed title) so less surprises when actually doing make lint

refack · 2017-04-29T16:45:57Z

(By the way: I assume it’s by accident, but you keep opening PRs from the nodejs/node repo instead of your fork … we don’t do that except for releases branches, can you try to make sure you open PRs from your fork for the future?)

Sorry, apparently it happens when I use the (limited) web interface.

aqrln

Can you also format the commit message according to the commit guidelines, please?
(s/adding/add).

aqrln · 2017-04-29T17:49:37Z

CONTRIBUTING.md

+   feature, etc. This should be in the commit messages as well as in the PR
+   description.
+2. #### Don't make _unnecessary_ code changes
+   Things that are changed because of personal preferance or style, like:


s/preferance/preference

aqrln · 2017-04-29T17:50:54Z

CONTRIBUTING.md

+   renaming of variables or functions, adding or removing white spaces,
+   reordering lines or whole code blocks. These sort of changes should have
+   a good reason since they cause unnecessary ["code churn"](https://blog.gitprime.com/why-code-churn-matters).
+   As part of the projects strategy we maintain several lines of code, code 


I believe something like "we maintain multiple release lines" would be more clear?

aqrln · 2017-04-29T17:53:14Z

CONTRIBUTING.md

+   use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide)
+   should 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 rules [custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules)  


This will turn into duplicate phrases when rendered. Making "our own custom rules" a single link instead of "our own rules" text followed by the "custom rules" link would be better.

refack · 2017-04-29T18:29:25Z

Addressed comments. PTAL.

Requested changes have been made.

addaleax

I still mostly think point 1 should be moved into the section that’s actually about commit messages, and I’m not sure points 2 and 3 need to be described in that much detail…

addaleax · 2017-04-29T18:37:40Z

CONTRIBUTING.md

+1. #### Provide motivation for the change
+   Why will this change make the code better; does it fix a bug, is it a new
+   feature, etc. This should be in the commit messages as well as in the PR
+   description.


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

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.

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

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

addaleax · 2017-04-29T18:38:07Z

CONTRIBUTING.md

+### 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


It might be helpful to use full sentences here

I tried, PTAL

addaleax · 2017-04-29T18:38:26Z

CONTRIBUTING.md

+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


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

addaleax · 2017-04-29T18:39:11Z

CONTRIBUTING.md

+   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.
+3. #### Keep your change-set self contained but at a reasonable size


typo: self-contained

addaleax · 2017-04-29T18:40:39Z

CONTRIBUTING.md

+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)
+   should keep you in line.  


“keep you in line”?

addaleax · 2017-04-29T18:40:45Z

CONTRIBUTING.md

+   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)


typo: adjustments

aqrln · 2017-04-29T18:55:23Z

@refack I dismissed that red cross sign, but not explicitly approving, I'm +0 on this. Generally it looks good but I have several concerns that overweight it.

Apparently, a significant amount of the first-time contributors either don't look at this document at all, however prominent it is and despite the link being shown when they open a PR, or just skim through it without really understanding it. Just look at all those 100 characters length capitalized commit messages in PRs with the "commit message follows commit guidelines" item being marked with a tick. I'd really love to see ways and ideas to make this information more accessible, not to overwhelm people with another wall of text.
I don't really think all of these points can be qualified as "rules of thumb". Some of them are arguable and really depend on the situation and some of them are not just rules of thumb but rather requirements that are better moved to appropriate places of the document, if there are ones. I don't think there's a section covering the code style, though, and I'd rather create it somewhere and populate it with information about the Google C++ Code Style, cpplint, our custom ESLint rules and how to run the linter, etc.

refack · 2017-04-29T19:00:20Z

@aqrln I assumed so. Let's let this PR stew (as PRs do). Anyway I hope this PR is a good start
I'll add you reservations as invitation for ideas in the first comment.

P.S. This is actually a fix to #12636

…mit message" section

Trott · 2017-04-29T19:27:55Z

I'd rather see less text, not more, in the CONTRIBUTING.md. I'd prefer a PR that removes everything that isn't absolutely essential.

If we're going to provide helpful hints like this, I'd prefer they be below the instructions for forking etc., rather than at the top of the doc.

I think most people will be discouraged by a wall of a text and won't read it. If we want to include this content, it should be one sentence per point with links to other docs if further explanation should be required.

refack · 2017-04-29T19:46:04Z

After @aqrln's @Trott's comments about "wall of text", I collapsed the explanations, so now it's a consise self-contained "rules" list

But IMHO even before it wasn't so bad...

Trott · 2017-04-29T19:48:10Z

CONTRIBUTING.md

@@ -122,6 +164,7 @@ changed and why. Follow these guidelines when writing one:
    Refs: http://eslint.org/docs/rules/space-in-parens.html
    Refs: https://github.com/nodejs/node/pull/3615
    ```
+    - It's it's not a fix, you should document the motivation for the change


It's it's -> If it's

Punctuation at the end of the change.

Avoid you.

So something like this perhaps:

If it's not a fix, document the motivation for the change.

Fixed and removed you, but there are a ton of previously existing your in there.

refack · 2017-04-29T19:57:11Z

I'd rather see less text, not more, in the CONTRIBUTING.md. I'd prefer a PR that removes everything that isn't absolutely essential.

If we're going to provide helpful hints like this, I'd prefer they be below the instructions for forking etc., rather than at the top of the doc.

I think most people will be discouraged by a wall of a text and won't read it. If we want to include this content, it should be one sentence per point with links to other docs if further explanation should be required.

I agree on less text but would go the other way, leave this guideline, and split technical stuff to another file. IMHO People should think a little bit beforehand, they can figure out the technical stuff later. (yeah, I heard that too. Funny that it is coming from me ☺️)

lpinca · 2017-05-02T09:29:11Z

CONTRIBUTING.md

+<details>
+<summary><strong>Provide motivation for the change</strong></summary>
+Try to explain why this change will make the code better. For example, does
+it fix a bug, or is it a new feature, etc. This should expressed in the 


Missing "be" in "This should be expressed".

lpinca · 2017-05-02T09:39:18Z

CONTRIBUTING.md

+Use good judgment when making a big change. If a reason doesn't come to mid
+but a very big chage needs to be made, try to break it into smaller
+pieces (still as self-contained as possible), and cross-reference them, and 
+marking some of them as `blocked` pending the others.   


I think that asking this to a first time contributor is a bit too much. In the unlikely case where they will propose a big change we can still ask them to apply the necessary tweaks.

I think only Collaborators can label issues, so the people who actually read this guide (that is: non-Collaborators) won't be able to do as instructed (mark something as blocked).

refack · 2017-05-02T17:35:29Z

@lpinca @Trott fixed your comments.
Have you any idea for a better place for this or is #12748 a good direction.

Trott · 2017-05-02T17:59:43Z

Have you any idea for a better place for this or is #12748 a good direction.

@refack I don't know what problem this is trying to solve. To the extent that I am aware of problems with the CONTRIBUTING.md doc, this would seem to exacerbate them.

This doc is mostly for new or infrequent contributors. While there are exceptions, the usual problem we have with these types of contributions is not (for example) that the change set lacks an obvious motivation. Yes, it happens, but it's not the biggest or most obvious pain point.

The problems we see frequently stem from the fact that people either don't read it or don't understand this document. So, for example, we get commits that don't adhere to the commit message guidelines. If we're trying to solve that problem (which would be laudable), I would say the solution is to make the material in the CONTRIBUTING.md more succinct and more scannable. Perhaps we can make it more visual (although there may be problems with that too).

I don't see any Collaborators (or anyone else for that matter) agreeing that either these changes or #12748 are needed or a good idea. I'm not even seeing positive GitHub reaction emoji things for either of them. I think the whole approach needs a re-think.

Documentation is difficult (and the difficulty is greatly exacerbated by people thinking it's not difficult but that's a long comment for another day). I don't have any concrete suggestions as to how to make the doc more concise and scannable, but that's what I'd focus on instead. (Oh, here's a concrete idea: Maybe a short table of contents or set of internal links near the top, just below the Code of Conduct text: If you're opening an issue, go here; if you're trying to contribute code, go here. Might help the doc feel less overwhelmingly long. Maybe. Just an idea.)

/cc @nodejs/documentation for some contrary/varied/other/concurring opinions/ideas

refack · 2017-05-02T18:11:41Z

@Trott I wasn't sure either that CONTRIBUTING.md is the right place for this, but IMHO these things should be put down in writing.

My motivation is that these "rules of thumb" or "best practices" are generally agreed upon, but the knowledge is not transferred or easily findable to new/infrequent contributors (in the wide sense not the @ nodejs/collaborators sense). I've refed some PRs that didn't consider these "rules" and so had to be addressed ad-hoc.

I agree the general mood for the PR is ambiguous, but the participation and commenting was fairly extensive, so I figure people agree with what's written, or in the least care 👍

refack · 2017-05-02T18:13:26Z

Added help-wanted and title to first comment.

refack · 2017-05-02T18:37:29Z

@Trott I'll stop editing this PR (it's on a bad branch anyway)
I moved the (well edited) content to #12791 which adds a new "catch all" guide.

I'm still looking for help in how to get the intended audience to read it.

MylesBorins · 2017-05-05T07:17:34Z

Two quick things 1) maybe a bunch of this content would make a good blog post for the nodejs medium 2) would prefer we avoided the term "rule of thumb"... Check out the Wikipedia article, but there is a chance some people may get the wrong idea

refack · 2017-05-05T18:47:05Z

would prefer we avoided the term "rule of thumb"... Check out the
Wikipedia article, but there is a chance some people may get the wrong idea

Ohh dear.
The current mood is to call it "Best Practices"

refack · 2017-05-05T21:41:36Z

maybe a bunch of this content would make a good blog post for the nodejs
medium

@MylesBorins who edits the medium? I could turn this list into a "how to submit your first PR" or "how to submit a good PR" post for editing.

MylesBorins · 2017-05-06T00:15:16Z

@refack you should make a post over in @nodejs/evangelism

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 29, 2017

vsemozhetbyt reviewed Apr 29, 2017

View reviewed changes

addaleax requested changes Apr 29, 2017

View reviewed changes

addaleax reviewed Apr 29, 2017

View reviewed changes

refack self-assigned this Apr 29, 2017

refack added the discuss Issues opened for discussions and feedbacks. label Apr 29, 2017

aqrln previously requested changes Apr 29, 2017

View reviewed changes

doc: add "rules of thumb" section

9a79fe6

refack force-pushed the refack-patch-2 branch from 999e1b6 to 9a79fe6 Compare April 29, 2017 18:27

[squash] add two missing '.'s

953c7e0

[squash] add some line breaks, and fix a few typos

9ab59c1

addaleax requested changes Apr 29, 2017

View reviewed changes

refack added 2 commits April 29, 2017 15:01

[squash] trying to form full sentences, and adding a line in the "com…

1691471

…mit message" section

[squash] more explicit working or style and linters

edc0b43

[squash] Collapse explanations

011263e

Trott reviewed Apr 29, 2017

View reviewed changes

Removing personal pronouns

3a9ee5b

[squash] its -> it is

fccb06a

refack mentioned this pull request Apr 29, 2017

doc:spin off technical guide #12748

Closed

2 tasks

lpinca reviewed May 2, 2017

View reviewed changes

[squash] typos & remove "mark as blocked"

3fd0eb2

refack added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 2, 2017

Trott mentioned this pull request May 2, 2017

doc: edit CONTRIBUTING.md for clarity etc. #12796

Closed

2 tasks

refack closed this May 5, 2017

refack deleted the refack-patch-2 branch May 5, 2017 17:16

refack mentioned this pull request May 5, 2017

doc: add "Troubleshooting" & "best practices" #12791

Closed

2 tasks

refack removed their assignment Jun 12, 2017

doc: adding "rules of thumb" #12744

doc: adding "rules of thumb" #12744

Conversation

refack commented Apr 29, 2017 • edited Loading

Discussion moved to #12791

[edit]

[/edit]

Open questions:

Checklist

Affected core subsystem(s)

vsemozhetbyt Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Apr 29, 2017

aqrln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Apr 29, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Apr 29, 2017

refack commented Apr 29, 2017

Trott commented Apr 29, 2017

refack commented Apr 29, 2017

Trott Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 2, 2017

Trott commented May 2, 2017

refack commented May 2, 2017

refack commented May 2, 2017

refack commented May 2, 2017

MylesBorins commented May 5, 2017 via email • edited Loading

refack commented May 5, 2017

refack commented May 5, 2017

MylesBorins commented May 6, 2017

refack commented Apr 29, 2017 •

edited

Loading

vsemozhetbyt Apr 29, 2017 •

edited

Loading

Trott Apr 29, 2017 •

edited

Loading

MylesBorins commented May 5, 2017 via email •

edited

Loading