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

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

sjlehn
Copy link
Contributor

@sjlehn sjlehn commented Apr 27, 2017

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
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
@nodejs-github-bot 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
doc/api/path.md Outdated
@@ -416,6 +416,16 @@ added: v0.11.15
The `path.posix` property provides access to POSIX specific implementations
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
Copy link
Member

Choose a reason for hiding this comment

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

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

doc/api/path.md Outdated
@@ -416,6 +416,16 @@ added: v0.11.15
The `path.posix` property provides access to POSIX specific implementations
of the `path` methods.

### path.posix.normalize(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sjlehn sjlehn Apr 27, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't give it a heading.

doc/api/path.md Outdated
@@ -416,6 +416,16 @@ added: v0.11.15
The `path.posix` property provides access to POSIX specific implementations
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 POSIX as a valid directory separator.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duh...should have realized I had that backwards

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of suggestions

doc/api/path.md Outdated

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

Choose a reason for hiding this comment

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

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

doc/api/path.md Outdated
@@ -330,6 +330,15 @@ For example on POSIX:
path.normalize('/foo/bar//baz/asdf/quux/..');
// Returns: '/foo/bar/baz/asdf'
```
*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.
Copy link
Member

Choose a reason for hiding this comment

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

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

`\` and `/`

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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
doc/api/path.md Outdated
For example:
```js
path.posix.normalize("\\..\\some\\thing\\like\\this")
//Returns '\..\some\thing\like\this'
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@vsemozhetbyt
Copy link
Contributor

Linter:

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

doc/api/path.md Outdated
@@ -330,6 +330,15 @@ For example on POSIX:
path.normalize('/foo/bar//baz/asdf/quux/..');
// Returns: '/foo/bar/baz/asdf'
```
*Note*: The `path.posix.normalize()` method will not attempt to convert `\ `
Copy link
Contributor

@sam-github sam-github Apr 27, 2017

Choose a reason for hiding this comment

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

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
doc/api/path.md Outdated
`/` 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
Copy link
Contributor

@sam-github sam-github Apr 28, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@sjlehn
Copy link
Contributor Author

sjlehn commented May 4, 2017

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

@addaleax
Copy link
Member

addaleax commented May 7, 2017

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!

@addaleax addaleax closed this May 7, 2017
addaleax pushed a commit that referenced this pull request May 7, 2017
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>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants