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

tools: fix json doc generate error caused by type loose_item_start #5943

Conversation

firedfox
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

  • tools

Description of change

fixes #5942

Current processList function in tools/doc/json.js does not recognise
{"type":"loose_item_start"}. Fix it.
@mscdex mscdex added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Mar 29, 2016
@Fishrock123
Copy link
Contributor

cc @nodejs/documentation

@silverwind
Copy link
Contributor

LGTM.

This loose_item_start type looks to have been introduced recently, but I can't seem to find the exact commit, any idea?

@firedfox
Copy link
Contributor Author

It's because of such content that appeared recently in doc:

## fs.readdir(path[, options], callback)

* `path` {String | Buffer}
* `options` {String | Object}
  * `encoding` {String} default = `'utf8'`
* `callback` {Function}

* `path` {String}
* `callback` {Function}

Such format produces loose_item_start:

## Heading

* item0

* item1
[{"type":"list_start","ordered":false},{"type":"loose_item_start"},{"type":"text","text":"item0"},{"type":"list_item_end"},{"type":"loose_item_start"},{"type":"text","text":"item1"},{"type":"space"},{"type":"list_item_end"},{"type":"list_end"}]

eljefedelrodeodeljefe referenced this pull request Mar 30, 2016
This makes several changes:

1. Allow path/filename to be passed in as a Buffer on fs methods
2. Add `options.encoding` to fs.readdir, fs.readdirSync, fs.readlink,
   fs.readlinkSync and fs.watch.
3. Documentation updates

For 1... it's now possible to do:

```js
fs.open(Buffer('/fs/foo/bar'), 'w+', (err, fd) => { });
```

For 2...
```js
fs.readdir('/fs/foo/bar', {encoding:'hex'}, (err,list) => { });

fs.readdir('/fs/foo/bar', {encoding:'buffer'}, (err, list) => { });
```

encoding can also be passed as a string

```js
fs.readdir('/fs/foo/bar', 'hex', (err,list) => { });
```

The default encoding is set to UTF8 so this addresses the
discrepency that existed previously between fs.readdir and
fs.watch handling filenames differently.

Fixes: #2088
Refs: #3519
PR-URL: #5616
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@eljefedelrodeodeljefe
Copy link
Contributor

@silverwind I think this is some weird merging mistake during some work on fs. Seems like the older commit was merged first and then git was adding the older more incomplete version as addition. In any case the docs at the above line number are just wrong and would need to be fixed. See comment on @jasnell commit above.

@firedfox's commit seems legit anyhow, since it's just adding the case if it was intentional. LGTM

@firedfox
Copy link
Contributor Author

Oh I thought those duplicated comments were added on purpose. Should I make another PR to remove them from doc?

@eljefedelrodeodeljefe
Copy link
Contributor

I am not sure, but it seems very likely.
Sure go ahead. If you don't want to waste your time you could also wait until tomorrow, until the sun has passed West USA for the folks to comment.

silverwind pushed a commit that referenced this pull request Mar 30, 2016
Current processList function in tools/doc/json.js does not recognise
{"type":"loose_item_start"}. Fix it.

PR-URL: #5943
Fixes: #5942
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@silverwind
Copy link
Contributor

Thanks! Landed in 05b3a0b.

@silverwind silverwind closed this Mar 30, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Current processList function in tools/doc/json.js does not recognise
{"type":"loose_item_start"}. Fix it.

PR-URL: #5943
Fixes: #5942
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@MylesBorins
Copy link
Contributor

is this something we want for lts?

@silverwind
Copy link
Contributor

@thealphanerd It fixes an edge case that manifested after #5616. I'd say it's nice to have in LTS as a safeguard.

evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Current processList function in tools/doc/json.js does not recognise
{"type":"loose_item_start"}. Fix it.

PR-URL: #5943
Fixes: #5942
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Current processList function in tools/doc/json.js does not recognise
{"type":"loose_item_start"}. Fix it.

PR-URL: #5943
Fixes: #5942
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error found when generating JSON doc
6 participants