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.

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: unify trailing commas in code examples #12557

Closed
wants to merge 1 commit into from
Closed

doc: unify trailing commas in code examples #12557

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 21, 2017

Checklist
Affected core subsystem(s)

doc

There were some trailing commas in code examples, so I've tried to unify this (in some places I've updated output examples according to actual output that is incompatible with trailing commas, just to be consistent).

This change may secure a bit possible future editing and may reduce diff noise in such cases (see ESLint rule).

Feel free to close if this is an overestimated cosmetic change.

@vsemozhetbyt vsemozhetbyt added the doc Issues and PRs related to the documentations. label Apr 21, 2017
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

I don't think we are using trailing commas in actual code, and I'm -1 on using a style in documentation that is different from our official style.

@@ -530,16 +530,14 @@ running the `./configure` script.
An example of the possible output looks like:

```js
{
target_defaults:
{ target_defaults:
Copy link
Member

Choose a reason for hiding this comment

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

This kind of style shift is actually against what the PR stands for, since the diff would be inflated if the first property was removed. (That's not to say I'm against this new style -- In fact I prefer it over the old one -- but consistency is key in documentation.)

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've changed these fragments as they are claimed to be the actual output — and this is the actual output. I prefer to update them than to leave inconsistent with other places with trailing commas.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

@TimothyGu Well, the rule does not forbid trailing commas. And they happen in our code:

node/lib/fs.js

Line 254 in 096508d

X_OK: {enumerable: true, value: constants.X_OK || 0},

message: message.msg,

message: message,

https://github.com/nodejs/node/blob/master/lib/internal/http.js#L9

+ in benchmarks: ~30 fragments
+ in tests: ~150 fragments

@not-an-aardvark
Copy link
Contributor

not-an-aardvark commented Apr 21, 2017

Would it be worth using eslint-plugin-markdown to lint our documentation for consistent JS style?

edit: I tried this out, results are here

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2017

@not-an-aardvark Currently, we have only 30 cases with Parsing error in doc/api (object literals erroneously parsed as code blocks; terminal or REPL interaction logs; uncommented ellipses; truncated constructions; joined conflicting contexts). This can be easily fixed.

eslint --no-eslintrc --ext md --plugin markdown --env es6,node --parser-options=ecmaVersion:2017 --parser-options=sourceType:script doc/api

Output (click me):
doc\api\child_process.md
  116:7   error  Parsing error: Identifier 'bat' has already been declared
  200:10  error  Parsing error: Unexpected token :
  367:6   error  Parsing error: Unexpected token :

doc\api\console.md
  72:7  error  Parsing error: Identifier 'Console' has already been declared

doc\api\dns.md
  299:10  error  Parsing error: Unexpected token :
  358:13  error  Parsing error: Unexpected token :
  388:9   error  Parsing error: Unexpected token :

doc\api\fs.md
  221:7   error  Parsing error: Unexpected token {
  635:11  error  Parsing error: Unexpected token :
  701:18  error  Parsing error: Unexpected token :

doc\api\http.md
  16:19  error  Parsing error: Unexpected token :

doc\api\modules.md
  561:16  error  Parsing error: Unexpected token

doc\api\os.md
  274:7  error  Parsing error: Unexpected token :

doc\api\process.md
   536:27  error  Parsing error: Unexpected token :
   752:8   error  Parsing error: Unexpected token :
  1180:12  error  Parsing error: Unexpected token :
  1351:6   error  Parsing error: Unexpected token :
  1716:7   error  Parsing error: Unexpected token :

doc\api\querystring.md
  65:6  error  Parsing error: Unexpected token :

doc\api\repl.md
   44:1  error  Parsing error: Unexpected token >
   79:1  error  Parsing error: Unexpected token >
  107:3  error  Parsing error: Unexpected token node
  136:1  error  Parsing error: Unexpected token >
  146:1  error  Parsing error: Unexpected token >
  292:4  error  Parsing error: Unexpected token /
  442:3  error  Parsing error: Unexpected token node

doc\api\tls.md
  1249:34  error  Parsing error: Unexpected token )

doc\api\v8.md
  122:29  error  Parsing error: Unexpected token :

doc\api\zlib.md
   91:7   error  Parsing error: Identifier 'zlib' has already been declared
  162:27  error  Parsing error: Unexpected token :

✖ 30 problems (30 errors, 0 warnings)

@jasnell
Copy link
Member

jasnell commented Apr 21, 2017

I'm generally not a fan of trailing commas but not enough to block this ;-)

@vsemozhetbyt
Copy link
Contributor Author

Closing for now in favor of #12563. If somebody has a strong approving, let me know and I shall address this after #12563 is settled.

@vsemozhetbyt vsemozhetbyt deleted the doc-comma-dangle branch April 21, 2017 21:05
@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

+1 to trailing commas everywhere

vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
This is an initial step to eliminate most of parsing errors.

PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
* install eslint-plugin-markdown
* add doc/.eslintrc.yaml
* add `<!-- eslint-disable rule -->` or `<!-- eslint-disable -->`
  for the rest of problematic code
* .js files in doc folder added to .eslintignore
* update Makefile and vcbuild.bat

PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
This is an initial step to eliminate most of parsing errors.

PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
evanlucas pushed a commit that referenced this pull request May 1, 2017
This is an initial step to eliminate most of parsing errors.

PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
This is an initial step to eliminate most of parsing errors.

PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
This is an initial step to eliminate most of parsing errors.

Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
This is an initial step to eliminate most of parsing errors.

Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
This is an initial step to eliminate most of parsing errors.

Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
Backport-PR-URL: #14067
PR-URL: #12563
Refs: #12557 (comment)
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants