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: do not begin yaml value with backtick #15447

Closed
wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

Will break YAML parsing otherwise!

Ref: #14930, cc @vsemozhetbyt


Full details about error:

Original Node error:
Jonathans-MBP:node jon$ node tools/doc/generate.js --format=json doc/api/dgram.md
Input file = doc/api/dgram.md
{ Error
    at generateError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at readBlockSequence (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:935:7)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1331:12)
    at readBlockMapping (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1062:11)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
    at readDocument (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
    at loadDocuments (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
  name: 'YAMLException',
  reason: 'bad indentation of a sequence entry',
  mark:
   Mark {
     name: null,
     buffer: '\nadded: v0.11.13\nchanges:\n  - version: REPLACEME\n    pr-url: https://github.com/nodejs/node/pull/14560\n    description: The `lookup` option is supported.\n  - version: REPLACEME\n    pr-url: https://github.com/nodejs/node/pull/13623\n    description: `recvBufferSize` and `sendBufferSize` options are supported now.\n\u0000',
     position: 248,
     line: 8,
     column: 17 },
  message: 'bad indentation of a sequence entry at line 9, column 18:\n        description: `recvBufferSize` and `sendBuffer ... \n                     ^' }

Then I extracted out the problematic YAML, and tried running through Ruby as a
separate .yml file:

Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 8 column 18
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse_stream'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:325:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:252:in `load'
	from (irb):7
	from /Users/jon/.rbenv/versions/2.4.0/bin/irb:11:in `<main>'

Not intimately familiar, but node-test-pull-request / node-test-commit should be compiling the docs somewhere along the line, right? 😬

Checklist
Affected core subsystem(s)

doc

Will break YAML parsing!

Original Node error:

```
Jonathans-MBP:node jon$ node tools/doc/generate.js --format=json doc/api/dgram.md
Input file = doc/api/dgram.md
{ Error
    at generateError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at readBlockSequence (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:935:7)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1331:12)
    at readBlockMapping (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1062:11)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
    at readDocument (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
    at loadDocuments (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
  name: 'YAMLException',
  reason: 'bad indentation of a sequence entry',
  mark:
   Mark {
     name: null,
     buffer: '\nadded: v0.11.13\nchanges:\n  - version: REPLACEME\n    pr-url: https://github.com/nodejs/node/pull/14560\n    description: The `lookup` option is supported.\n  - version: REPLACEME\n    pr-url: https://github.com/nodejs/node/pull/13623\n    description: `recvBufferSize` and `sendBufferSize` options are supported now.\n\u0000',
     position: 248,
     line: 8,
     column: 17 },
  message: 'bad indentation of a sequence entry at line 9, column 18:\n        description: `recvBufferSize` and `sendBuffer ... \n                     ^' }
```

Then I extracted out the problematic YAML, and tried running through Ruby as a
separate `.yml` file:

```
Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 8 column 18
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse_stream'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:325:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:252:in `load'
	from (irb):7
	from /Users/jon/.rbenv/versions/2.4.0/bin/irb:11:in `<main>'
```
@nodejs-github-bot nodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations. labels Sep 17, 2017
@maclover7 maclover7 mentioned this pull request Sep 17, 2017
@refack
Copy link
Contributor

refack commented Sep 17, 2017

Nice work!

Not intimately familiar, but 1node-test-pull-request1 / 1node-test-commit1 should be compiling the docs somewhere along the line, right? 😬

Nope.
That's a very good test to add (running node tools/doc/generate.js), at first probably checking sanity, but later maybe validating some invariants, a la test-benckmark-*.js

@vsemozhetbyt
Copy link
Contributor

Can this be handled by md linting? Maybe by some YAML plugin?

@refack
Copy link
Contributor

refack commented Sep 17, 2017

Can this be handled by md linting? Maybe by some YAML plugin?

👍 that was my other thought, but IMHO anyway end2end testing of tools/doc/generate.js should happen as part of regular CI...

@vsemozhetbyt
Copy link
Contributor

Should this wait 48 hours?

@addaleax
Copy link
Member

@vsemozhetbyt I don’t think it needs to, no

@vsemozhetbyt
Copy link
Contributor

I will land then)

vsemozhetbyt pushed a commit that referenced this pull request Sep 17, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in 75f7b2f.
I've simplified the commit message, as all the details are in the PR.
Let's see if the docs appear in the nearest Nightly.

@vsemozhetbyt
Copy link
Contributor

@maclover7 Thank you for finding out the cause and fixing!

@vsemozhetbyt
Copy link
Contributor

Seems fixed: https://nodejs.org/download/nightly/v9.0.0-nightly2017091775f7b2f577/docs/api/

addaleax pushed a commit to ayojs/ayo that referenced this pull request Sep 17, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: nodejs/node#15447
Fixes: nodejs/node#14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
Will break YAML parsing!

See details in the PR.

PR-URL: #15447
Fixes: #14930
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

commit that broke stuff didn't land on v6.x, setting this to do-not-land-v6.x, lmk if we should reconsider

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants