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

Jump to bottom

doc: remove non-style information from style guide #17866

Closed
wants to merge 4 commits into from
Closed

doc: remove non-style information from style guide #17866

wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 25, 2017

While I agree that tools should be used insofar as possible, that
information does not belong in the style guide.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@Trott Trott added the doc Issues and PRs related to the documentations. label Dec 25, 2017
BridgeAR
BridgeAR reviewed Dec 26, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

In general I agree but I personally would like this to be moved into e.g. the onboarding part instead of being removed.

@mmarchini
Copy link
Contributor

I agree with @BridgeAR, it should probably be moved to another doc. Maybe something like this in CONTRIBUTING.md (this way new Contributors will see it)?

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index d27959c6dc..987ae84bcd 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -284,6 +284,10 @@ should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included
 in the API docs will also be checked when running `make lint` (or
 `vcbuild.bat lint` on Windows).

+Mechanical issues, like spelling and grammar, should be identified by tools,
+insofar as is possible. If not caught by a tool, they should be pointed out by
+human reviewers.
+
 For contributing C++ code, you may want to look at the
 [C++ Style Guide](CPP_STYLE_GUIDE.md).

@Trott
Copy link
Member Author

Trott commented Dec 27, 2017

I agree with @BridgeAR, it should probably be moved to another doc. Maybe something like this in CONTRIBUTING.md (this way new Contributors will see it)?

I don't think CONTRIBUTING.md is the place for it. The audience for that document is people opening pull requests and issues. The correct audience here would be Collaborators and maybe a small number of other reviewers. Therefore, onboarding.md or (better IMO) COLLABORATOR_GUIDE.md.

I'm not going to fight to keep this paragraph out of our repository, but I honestly don't actually think the text has much value. It's an aspirational thing that is generally agreed upon as a best practice in software. It's basically "tools, not rules" which is widely regarded as a best practice. Including it isn't much different than including DRY, KISS, and YAGNI in my opinion. We don't include those things, and we shouldn't include "tools, not rules".

Now...ALL THAT SAID...I think maybe what really needs to happen is this line just needs to be changed to instruct people to run make lint-md on their doc changes.

@Trott
Copy link
Member Author

Trott commented Dec 27, 2017

Updated. PTAL!

@gibfahn
Copy link
Member

gibfahn commented Dec 28, 2017

I'm not going to fight to keep this paragraph out of our repository, but I honestly don't actually think the text has much value. It's an aspirational thing that is generally agreed upon as a best practice in software. It's basically "tools, not rules" which is widely regarded as a best practice. Including it isn't much different than including DRY, KISS, and YAGNI in my opinion. We don't include those things, and we shouldn't include "tools, not rules".

I think the idea is to suggest to reviewers that they avoid leaving style nits for things that we don't have lint rules for. Emphasizing DRY isn't necessary, because we have code reviews to do that (and it's too complex to be exact about). But who reviews the reviewers? Given the number of complaints we've had where new contributors were disheartened by the volume of nits, noting this in the Collaborator Guide seems worthwhile to me.

@Trott
Copy link
Member Author

Trott commented Dec 28, 2017

I think the idea is to suggest to reviewers that they avoid leaving style nits for things that we don't have lint rules for. Emphasizing DRY isn't necessary, because we have code reviews to do that (and it's too complex to be exact about). But who reviews the reviewers? Given the number of complaints we've had where new contributors were disheartened by the volume of nits, noting this in the Collaborator Guide seems worthwhile to me

Ah, yes, that makes a lot of sense. Thanks for the clarification/explanation.

@BridgeAR
Copy link
Member

@Trott as @gibfahn pointed out the original intention of this: would you be fine to move the original statement to the onboarding part? I still think it makes sense there.

@Trott
Copy link
Member Author

Trott commented Dec 28, 2017

would you be fine to move the original statement to the onboarding part?

Yeah, I can do that.

Trott added 3 commits December 29, 2017 16:02
@Trott
doc: remove non-style information from style guide
23fd08e 
While I agree that tools should be used insofar as possible, that
information does not belong in the style guide.
@Trott
squash
295ae9b
@Trott
squash: add note to onboarding doc
beaafb3
@Trott
Copy link
Member Author

Trott commented Dec 30, 2017

@BridgeAR et al.: I've added a comment to onboarding.md. PTAL.

@Trott
Copy link
Member Author

Trott commented Jan 1, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/60/

Trott added a commit to Trott/io.js that referenced this pull request Jan 3, 2018
@Trott
doc: remove non-style information from style guide
f51067a 
While tools should be used insofar as possible, that information
does not belong in the style guide. Move to onboarding doc.

PR-URL: nodejs#17866
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jan 3, 2018

Landed in f51067a

@Trott Trott closed this Jan 3, 2018
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@Trott @MylesBorins
doc: remove non-style information from style guide
aea5ced 
While tools should be used insofar as possible, that information
does not belong in the style guide. Move to onboarding doc.

PR-URL: #17866
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Trott @MylesBorins
doc: remove non-style information from style guide
aeb16bd 
While tools should be used insofar as possible, that information
does not belong in the style guide. Move to onboarding doc.

PR-URL: #17866
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Trott @MylesBorins
doc: remove non-style information from style guide
16fbd57 
While tools should be used insofar as possible, that information
does not belong in the style guide. Move to onboarding doc.

PR-URL: #17866
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

this didn't land cleanly on v6.x or v8.x so I opted to not land. Please feel free to change labels and open a backport

@Trott Trott deleted the style-guide-edit branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@XadillaX XadillaX XadillaX approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.