Skip to content

doc: add more info to fs.Dir and fix typos - #29890

Closed
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:fs-dir-docs-amendments
Closed

doc: add more info to fs.Dir and fix typos#29890
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:fs-dir-docs-amendments

Conversation

@Fishrock123

@Fishrock123Fishrock123 commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

Some doc bits / fixes which were missing from
cbd8d71

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

@Fishrock123Fishrock123 added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 8, 2019
Trott
Trott reviewed Oct 9, 2019
View reviewed changes
Comment threaddoc/api/fs.md Outdated
Trott
Trott reviewed Oct 9, 2019
View reviewed changes
Comment threaddoc/api/fs.md Outdated
Trott
Trott reviewed Oct 9, 2019
View reviewed changes
Comment threaddoc/api/fs.md Outdated
Trott
Trott approved these changes Oct 9, 2019
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some relatively trivial comments, but would be fine if this lands as is, as it's definitely an improvement.

@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

Left some relatively trivial comments, but would be fine if this lands as is, as it's definitely an improvement.

(Er, well, the 80-char wrapping will almost certainly have to happen unless the linter surprises me here....)

@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3973/

@Trott

Trott commented Oct 9, 2019•
edited
Loading

Copy link
Copy Markdown
Member

And...nope...the linter didn't complain about that long line.... should still be wrapped.... strange, though. If I add a line of all text that is longer than 80 chars, it complains. Maybe the new lint config skips over lines with links or something unexpected-to-me like that? /ping @nschonni

@nschonni

nschonni commented Oct 9, 2019

Copy link
Copy Markdown
Member

Looks like it doesn't count code content and links https://github.com/remarkjs/remark-lint/blob/1959381435f94ea0107df7e4242a969c1c55f2ff/packages/remark-lint-maximum-line-length/index.js#L114-L115

@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

Looks like it doesn't count code content and links https://github.com/remarkjs/remark-lint/blob/1959381435f94ea0107df7e4242a969c1c55f2ff/packages/remark-lint-maximum-line-length/index.js#L114-L115

So any line that includes a link or inline code will not be checked for length? Oof. (Although I get it--URLs in particular can be longer than 80 chars just by themselves, so....)

@nschonni

nschonni commented Oct 9, 2019

Copy link
Copy Markdown
Member

It does check the length, but it ignores code blocks towards the total character count. If you add a few more to that line, then it will flag it. It doesn't look like there is an option to un-ignore those sections

BridgeAR
BridgeAR approved these changes Oct 9, 2019
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the nits addressed.

@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

It does check the length, but it ignores code blocks towards the total character count. If you add a few more to that line, then it will flag it. It doesn't look like there is an option to un-ignore those sections

Without the code blocks, that line is still way over 80 characters. Without checking, I wonder if it's a bug where it's ignoring everything between the first and last ` rather than considering each matching pair of ``` ` ```` separately?

@Fishrock123Fishrock123 mentioned this pull request Oct 9, 2019
Closed
3 tasks
@Fishrock123
Fishrock123force-pushed the fs-dir-docs-amendments branch from f1bbc20 to 01d2a0bCompareOctober 9, 2019 18:42
@Fishrock123Fishrock123 added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 9, 2019
@Fishrock123

Fishrock123 commented Oct 9, 2019

Copy link
Copy Markdown
ContributorAuthor

Fast tracking this would be desirable if possible since there is already another conflicting docs PR: #29906

(upvote this)

@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3975/

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 9, 2019
@Trott

Trott commented Oct 9, 2019

Copy link
Copy Markdown
Member

Argh, needs a rebase.

@Fishrock123
doc: add more info to fs.Dir and fix typos
02bc272
Some doc bits / fixes which were missing from
cbd8d71
@Fishrock123
Fishrock123force-pushed the fs-dir-docs-amendments branch from 01d2a0b to 02bc272CompareOctober 9, 2019 22:17
@Fishrock123

Fishrock123 commented Oct 9, 2019

Copy link
Copy Markdown
ContributorAuthor

Lite CI, post-rebase: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3979/

nschonni
nschonni reviewed Oct 9, 2019
View reviewed changes
Comment threaddoc/api/fs.md

Created by [`fs.opendir()`][], [`fs.opendirSync()`][], or [`fsPromises.opendir()`][].

Example using async interation:

@nschonninschonniOct 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the typo in this one be fixed too instead of removing the line?

@TrottTrottOct 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the line should be removed. In general, I dislike putting for example: and similar stuff before code that is obviously an example.

Trott pushed a commit that referenced this pull request Oct 10, 2019
@Fishrock123@Trott
doc: add more info to fs.Dir and fix typos
b41989d
Some doc bits / fixes which were missing from
cbd8d71
PR-URL: #29890
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

Trott commented Oct 10, 2019

Copy link
Copy Markdown
Member

Landed in b41989d

@TrottTrott closed this Oct 10, 2019
@Fishrock123
Fishrock123 deleted the fs-dir-docs-amendments branch October 10, 2019 07:15
BridgeAR pushed a commit that referenced this pull request Oct 10, 2019
@Fishrock123@BridgeAR
doc: add more info to fs.Dir and fix typos
20b9ef9
Some doc bits / fixes which were missing from
cbd8d71
PR-URL: #29890
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeARBridgeAR mentioned this pull request Oct 10, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@nschonninschonninschonni left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Fishrock123@Trott@nschonni@jasnell@addaleax@lpinca@cjihrig@BridgeAR