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

Closed
JestDotty opened this issue Apr 10, 2017 · 10 comments
Closed
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@JestDotty
Copy link

JestDotty commented Apr 10, 2017

  • Version: 4.4.2
  • Platform: Windows 8.1 64 bit
  • Subsystem: node.js native path module

console.log('1', path.posix.normalize("\\some\\thing\\like\\this"))
console.log('2', path.posix.normalize("/some/thing/like/this"))
console.log('3', path.win32.normalize("\\some\\thing\\like\\this"))
console.log('4', path.win32.normalize("/some/thing/like/this"))

output:
image

1 output should be the same as 2s though
Docs need to explain why this is the expected output

@vsemozhetbyt vsemozhetbyt added the path Issues and PRs related to the path subsystem. label Apr 10, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 10, 2017

If I get this right, win32 methods can process both separators, while posix methods don't consider \ a valid separator. Compare how other win32 methods parse paths with / properly, while the posix methods do not this for \:

const path = require('path');

const win32ToPosix = '\\win32\\to\\posix';
const posixToWin32 = '/posix/to/win32';

console.log(`
  ${path.posix.basename(win32ToPosix)}
  ${path.win32.basename(posixToWin32)}

  ${path.posix.dirname(win32ToPosix)}
  ${path.win32.dirname(posixToWin32)}

  ${path.posix.normalize(win32ToPosix)}
  ${path.win32.normalize(posixToWin32)}

  ${JSON.stringify(path.posix.parse(win32ToPosix))}
  ${JSON.stringify(path.win32.parse(posixToWin32))}

  ${path.posix.resolve(win32ToPosix)}
  ${path.win32.resolve(posixToWin32)}
`);
c:\>node test.js

  \win32\to\posix
  win32

  .
  /posix/to

  \win32\to\posix
  \posix\to\win32

  {"root":"","dir":"","base":"\\win32\\to\\posix","ext":"","name":"\\win32\\to\\posix"}
  {"root":"/","dir":"/posix/to","base":"win32","ext":"","name":"win32"}

  c:\/\win32\to\posix
  c:\posix\to\win32

c:\>

@bnoordhuis
Copy link
Member

Answered. Closing, working as intended.

@JestDotty
Copy link
Author

JestDotty commented Apr 10, 2017

the docs say:

When multiple, sequential path segment separation characters are found (e.g. / on POSIX and \ on Windows), they are replaced by a single instance of the platform specific path segment separator. Trailing separators are preserved.

which is a little weird so I skimmed it with my understanding, but for postarity this this doesn't work either:

console.log('1', path.posix.normalize("\\\\some\\\\thing\\\\like\\\\this"))
console.log('2', path.posix.normalize("/\\some/\\thing/\\like/\\this"))

image

To be fair it is vague enough to have concluded to my understanding... but on a more conceptual/abstract/module angle a conversion looking like this is intended / not available in a path manipulation library or am I missing something?

path.normalize(file).split(/[\\\/]/g).join(path.posix.sep)

@addaleax
Copy link
Member

@Seudein On POSIX, \\ is not a path separator, so I think path.posix.normalize is right in that /\ shouldn’t get contracted into a single /… what would your expected output for that be?

@refack
Copy link
Contributor

refack commented Apr 10, 2017

Maybe we need a posix2windows / windows2posix methods

@addaleax
Copy link
Member

@refack Sorry, could you explain how those methods would behave?

@refack
Copy link
Contributor

refack commented Apr 10, 2017

@refack Sorry, could you explain how those methods would behave?

posix2windows would assume the input is a posix path and flip the /
(then do some voodoo to try and find on which drive the file exists image [or not])

windows2posix would assume that the input is windows and make sure all \ are turned into /

I think what @Seudein assumed was that using path.posix.* on a windows platform will act like windows2posix


Answered. Closing, working as intended.

@bnoordhuis I believe that if a user open an issue based on wrong assumptions that are not addressed in the docs, it should be treated as a bug in the docs.

@refack refack added doc Issues and PRs related to the documentations. and removed path Issues and PRs related to the path subsystem. labels Apr 10, 2017
@refack refack changed the title path.posix.normalize does not replace windows slashes docs: explain why path.posix.normalize does not replace windows slashes Apr 10, 2017
@refack refack added the good first issue Issues that are suitable for first-time contributors. label Apr 10, 2017
@refack refack reopened this Apr 10, 2017
@refack
Copy link
Contributor

refack commented Apr 10, 2017

Reopened as a doc bug

@addaleax
Copy link
Member

then do some voodoo to try and find on which drive the file exists

path methods generally don’t actually query the file system, I would like to keep it that way.

@refack
Copy link
Contributor

refack commented Apr 10, 2017

I would like to keep it that way.

I agree. Super important!
I was sarcastic, I was hoping the huge emoji and [or not] would convey that, sorry...

@addaleax addaleax closed this as completed May 7, 2017
addaleax pushed a commit that referenced this issue 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 issue 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>
gibfahn pushed a commit that referenced this issue 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 issue 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>
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. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

5 participants