doc: adding "rules of thumb" by refack · Pull Request #12744 · nodejs/node

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

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

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

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

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

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

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

s/preferance/preference

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

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

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

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

+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

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

+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

+   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

+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

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

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

Comment thread CONTRIBUTING.md Outdated

Copy link
Copy Markdown

Contributor

vsemozhetbyt Apr 29, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

but -> bug

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

Uh oh!

Conversation

refack commented Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Discussion moved to #12791

[edit]

[/edit]

Open questions:

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 29, 2017

Uh oh!

aqrln left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 29, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aqrln commented Apr 29, 2017

Uh oh!

refack commented Apr 29, 2017

refack commented Apr 29, 2017 •

edited

Loading

vsemozhetbyt Apr 29, 2017 •

edited

Loading