Skip to content

Conversation

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbytvsemozhetbyt 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.

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

@TimothyGuTimothyGu 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.

```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
ContributorAuthor

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
ContributorAuthor

vsemozhetbyt commented Apr 21, 2017

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

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
ContributorAuthor

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

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

@vsemozhetbyt
Copy link
ContributorAuthor

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.

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

+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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
vsemozhetbyt added a commit that referenced this pull request Apr 24, 2017
PR-URL: #12563 Refs: #12557 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@vsemozhetbyt@not-an-aardvark@jasnell@gibfahn@TimothyGu