docs: explain why path.posix.normalize does not replace windows slashes by sjlehn · Pull Request #12700 · nodejs/node

sjlehn · 2017-04-27T16:24:34Z

Add section to path docs that explains that path.posix.normalize
does not replace Windows slashes with POSIX slashes because POSIX
does not recognize \ as a valid path separator.

Fixes: #12298

Added a note about directory separator conversion under path.posix.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Add section to path docs that explains that path.posix.normalize does not replace Windows slashes with POSIX slashes because POSIX does not recognize / as a valid path separator. Fixes: #12298

jasnell · 2017-04-27T16:25:33Z

doc/api/path.md

 of the `path` methods.

+### path.posix.normalize(path)
+The `path.posix.normalize()` method will not attempt to convert / (Windows) to \ (POSIX), as / is not recognized by 


long line here. Please line break at 80 chars :-)

Fix formatting (long line) Fixes: #12298

cjihrig · 2017-04-27T16:29:43Z

doc/api/path.md

 The `path.posix` property provides access to POSIX specific implementations
 of the `path` methods.

+### path.posix.normalize(path)


I think this note belongs in the normalize() documentation, not as a different subsection in path.posix.

Should I still give it a heading or would it be better as just a note after "For Example, on POSIX" at 327

I wouldn't give it a heading.

cjihrig · 2017-04-27T16:31:34Z

doc/api/path.md


+### path.posix.normalize(path)
+The `path.posix.normalize()` method will not attempt to convert / (Windows) to 
+\ (POSIX), as / is not recognized by POSIX as a valid directory separator.  


Isn't / a valid directory separator on POSIX??

duh...should have realized I had that backwards

I think the comment may have these backwards. The windows separator \ is not recognized by POSIX as a valid separator.

Move the comment up to the main path.normalize section and fix the mix-up with / and \ Fixes: #12298

jasnell

LGTM with a couple of suggestions

jasnell · 2017-04-27T16:45:40Z

doc/api/path.md

+
+For example:
+```js
+path.posix.normalize("\\some\\thing\\like\\this")


Suggestion: include .. in the sample path to show that those segments are not removed from the normalized path.

jasnell · 2017-04-27T16:46:08Z

doc/api/path.md

 ```
+*Note*: The `path.posix.normalize()` method will not attempt to convert \ 
+(Windows) to / (POSIX), as \ is not recognized by POSIX as a valid directory 
+separator.  


Suggestion: wrap the \ and / in backticks...

`\` and `/`

cjihrig

LGTM with @jasnell's comments addressed.

Couple more formatting changes as requested in review: add backticks to slashes, and add .. to example path. Fixes: #12298

sam-github · 2017-04-27T17:01:56Z

doc/api/path.md

+For example:
+```js
+path.posix.normalize("\\..\\some\\thing\\like\\this")
+//Returns '\..\some\thing\like\this'


It does not return that string:

> path.posix.normalize("\\..\\some\\thing\\like\\this") '\\..\\some\\thing\\like\\this'

Either remove the string quotes, or leave the string quotes and properly escape the backslashes (I suggest the latter)

vsemozhetbyt · 2017-04-27T17:04:13Z

Typo in the PR and first commit message: POSIX does not recognize / as a valid path separator (should be \).

vsemozhetbyt · 2017-04-27T17:05:26Z

Linter:

  339:22  error  Strings must use singlequote  quotes
  339:50  error  Missing semicolon             semi

sam-github · 2017-04-27T17:13:00Z

doc/api/path.md

 path.normalize('/foo/bar//baz/asdf/quux/..');
 // Returns: '/foo/bar/baz/asdf'
 ```
+*Note*: The `path.posix.normalize()` method will not attempt to convert `\ ` 


This note is backwards.

The docs clearly state normalize does two things: resolving . and .. segments, and collapsing multiple path seperators. Adding a statement about what it does NOT do is just strange.

The real problem is that the docs are wrong. On Windows, both / and \ are valid directory seperators (the docs are wrong here), and all sequences of 1 or more valid path seperator are replaced with a single preferred path seperator.

For POSIX, there is only one path seperator, so the set of valid and preferred is identical.

Fow Windows, it needs a special note, there are two valid seperators, but one is preferred, so seperators may actually be changed during normalize:

> path.win32.normalize("hi////there\\\\/\\/\\\/you/there") 'hi\\there\\you\\there'

Besides being documented, it would also be useful to have a couple examples.

Reword note to describe what the method does, rather than what it doesn't do, per PR feedback Fixes: #12298

sam-github · 2017-04-28T17:19:04Z

doc/api/path.md

-`/` on POSIX and `\` on Windows), they are replaced by a single instance of the
-platform specific path segment separator. Trailing separators are preserved.
+`/` on POSIX and either `\` or `/` on Windows), they are replaced by a single 
+instance of the platform specific path segment separator. Trailing separators 


segment separator, path.sep.

windows has 2, the text above would allow normalize to replace all backslashes with a forwardslash on windows :-)

I suggest the link above

So you're saying I should clarify what "platform specific path segment operator" means in each case?

yes. you just said windows has two seperators, then said sequences are replaced by a single instance, but since there are two, which one does it get replace with? its described further down, but best make clear here.

Clarify "platform specific separator" Fixes: #12298

sjlehn · 2017-05-04T20:01:37Z

@sam-github updated. Anything else I should review, or is it good?

addaleax · 2017-05-07T21:22:50Z

Landed in 9caf78fbaa573bf65f6bbfb27643c072d724481a, fixed up linter errors and commit message while landing. Thanks for the PR! :)

edit: oops, forgot to git add the linter fixes. re-landed in cbd6fde, sorry!

Add section to path docs that explains that path.posix.normalize does not replace Windows slashes with POSIX slashes because POSIX does not recognize / as a valid path separator. Fixes: #12298 PR-URL: #12700 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Cai <davidcai1993@yahoo.com>

Add section to path docs that explains that path.posix.normalize does not replace Windows slashes with POSIX slashes because POSIX does not recognize / as a valid path separator. Fixes: nodejs#12298 PR-URL: nodejs#12700 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Cai <davidcai1993@yahoo.com>

Add section to path docs that explains that path.posix.normalize does not replace Windows slashes with POSIX slashes because POSIX does not recognize / as a valid path separator. Fixes: #12298 PR-URL: #12700 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Cai <davidcai1993@yahoo.com>

docs: explain why path.posix.normalize does not replace windows slashes

c6f50de

Add section to path docs that explains that path.posix.normalize does not replace Windows slashes with POSIX slashes because POSIX does not recognize / as a valid path separator. Fixes: #12298

nodejs-github-bot added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Apr 27, 2017

jasnell requested changes Apr 27, 2017

View reviewed changes

docs: explain why path.posix.normalize does not replace windows slashes

91bb7fa

Fix formatting (long line) Fixes: #12298

cjihrig requested changes Apr 27, 2017

View reviewed changes

cjihrig reviewed Apr 27, 2017

View reviewed changes

docs: explain why path.posix.normalize does not replace windows slashes

cfaeba4

Move the comment up to the main path.normalize section and fix the mix-up with / and \ Fixes: #12298

jasnell approved these changes Apr 27, 2017

View reviewed changes

cjihrig approved these changes Apr 27, 2017

View reviewed changes

docs: explain why path.posix.normalize does not replace windows slashes

b4eda57

Couple more formatting changes as requested in review: add backticks to slashes, and add .. to example path. Fixes: #12298

sam-github suggested changes Apr 27, 2017

View reviewed changes

docs: explain why path.posix.normalize does not replace windows slashes

7dd712f

Reword note to describe what the method does, rather than what it doesn't do, per PR feedback Fixes: #12298

sam-github reviewed Apr 28, 2017

View reviewed changes

docs: explain why path.posix.normalize does not replace windows slashes

cb7d451

Clarify "platform specific separator" Fixes: #12298

sam-github approved these changes May 5, 2017

View reviewed changes

addaleax approved these changes May 7, 2017

View reviewed changes

addaleax closed this May 7, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the land-on-v6.x label Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

jgierer12 mentioned this pull request Jan 6, 2018

Some paths in webpack configs aren't matched on Windows preactjs/preact-cli#453

Closed

Xenonym mentioned this pull request Dec 20, 2018

Add serve --one-page option to live preview a single page MarkBind/markbind#475

Merged

Uh oh!

Conversation

sjlehn commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjlehn Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 27, 2017

Uh oh!

vsemozhetbyt commented Apr 27, 2017

Uh oh!

sam-github Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjlehn commented May 4, 2017

Uh oh!

addaleax commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sjlehn commented Apr 27, 2017 •

edited

Loading

sjlehn Apr 27, 2017 •

edited

Loading

sam-github Apr 27, 2017 •

edited

Loading

sam-github Apr 28, 2017 •

edited

Loading

addaleax commented May 7, 2017 •

edited

Loading