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

 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

 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


+### 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

+
+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

 ```
+*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

+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

 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

-`/` 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!

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!

sjlehnApr 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-githubApr 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-githubApr 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

sjlehnApr 27, 2017•
edited
Loading

sam-githubApr 27, 2017•
edited
Loading

sam-githubApr 28, 2017•
edited
Loading

addaleax commented May 7, 2017•
edited
Loading