Skip to content

doc: imp phrase in fs.md - #40255

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
warlock1996:patch-3
Oct 9, 2021
Merged

doc: imp phrase in fs.md#40255
aduh95 merged 1 commit into
nodejs:masterfrom
warlock1996:patch-3

Conversation

@warlock1996

Copy link
Copy Markdown
Contributor

i think it is confusing, it is more clear now

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Sep 29, 2021

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the change is correct, AFAICT it would result in an error. I think there are three possibilities for file: URL on Windows:

  • have a non empty host name: the file: URL represents a UNC path

    node/doc/api/fs.md

    Lines 6760 to 6762 in d9ebc04

    // - WHATWG file URLs with hostname convert to UNC path
    // file://hostname/p/a/t/h/file => \\hostname\p\a\t\h\file
    readFileSync(newURL('file://hostname/p/a/t/h/file'));
  • have an empty host name and specify a drive letter: the file: represents a local path

    node/doc/api/fs.md

    Lines 6764 to 6766 in d9ebc04

    // - WHATWG file URLs with drive letters convert to absolute path
    // file:///C:/tmp/hello => C:\tmp\hello
    readFileSync(newURL('file:///C:/tmp/hello'));
  • have neither: throws ERR_INVALID_FILE_URL_PATH

    node/doc/api/fs.md

    Lines 6768 to 6771 in d9ebc04

    // - WHATWG file URLs without hostname must have a drive letters
    readFileSync(newURL('file:///notdriveletter/p/a/t/h/file'));
    readFileSync(newURL('file:///c/p/a/t/h/file'));
    // TypeError [ERR_INVALID_FILE_URL_PATH]: File URL path must be absolute

I agree that the phrasing could be improved though.

@warlock1996

Copy link
Copy Markdown
ContributorAuthor

@aduh95 yes it should be improved , also i think if we can add bullets for all three statements it will be very clear

@aduh95

Copy link
Copy Markdown
Contributor

Do you want to update this PR to add those bullet points to the docs?

@tniessen

Copy link
Copy Markdown
Member

I don't think bullet points are necessary. There is example code below and the first sentence is quite clear. I agree that the second sentence could be more clear though.

Comment threaddoc/api/fs.md Outdated

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

Thank you @warlock1996!

@warlock1996
warlock1996force-pushed the patch-3 branch 2 times, most recently from 49b0d96 to 8b35a40CompareOctober 4, 2021 13:45
@Ayase-252Ayase-252 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 7, 2021
@warlock1996

warlock1996 commented Oct 9, 2021

Copy link
Copy Markdown
ContributorAuthor

@aduh95 please merge and close !

PR-URL: nodejs#40255
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95

Copy link
Copy Markdown
Contributor

Landed in 0d50dfd

@aduh95
aduh95 merged commit 0d50dfd into nodejs:masterOct 9, 2021
targos pushed a commit that referenced this pull request Oct 13, 2021
PR-URL: #40255
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Oct 14, 2021
2 tasks
@richardlaurichardlau mentioned this pull request Oct 18, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@warlock1996@aduh95@tniessen@jasnell@britoederr@Ayase-252@nodejs-github-bot