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

path (extname, parse) cannot handle extension correctly for directory #6229

Closed
francisl opened this issue Apr 15, 2016 · 8 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.

Comments

@francisl
Copy link

  • Version: v5.10.1
  • Platform: Darwin Kernel Version 15.3.0
  • Subsystem: path

On OS X and Linux its perfectly legal to use dot '.' in directory name.
Currently path.extname and path.parse handle . in directory name as a file extension.

e.g.

ext.extname('/Users/Bob.Dev')
> '.Dev'
ext.parse('/Users/John.Smith')
> { root: '/',
  dir: '/Users',
  base: 'Bob.Dev',
  ext: '.Dev',
  name: 'Bob' }

I understand that path don't do any validation. But this behavior is confusing an error prone.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

the challenge here is that the path module does not know that it's parsing a directory vs. a file. There's really nothing that can be done here other than perhaps documenting the limitation. /cc @nodejs/documentation

@jasnell jasnell added path Issues and PRs related to the path subsystem. docs-requested labels Apr 15, 2016
@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

This is why in general it's typically a good idea to append a trailing path separator to indicate a directory. However that won't help with parsing this path (which could possibly be viewed as a bug).

@francisl
Copy link
Author

if you put a slash at the end, you will have the same results.

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

Right, I noted that. It could perhaps be viewed as a bug in that case shrug.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

Actually, that does sound like a bug. /foo/bar/ and /foo/bar are two different paths and yet path.parse returns the same results for each. (quick test confirms that this is the way it works in v4 also... sigh... definitely should get that documented then)

@jasnell jasnell added the confirmed-bug Issues with confirmed bugs. label Aug 27, 2016
jasnell added a commit to jasnell/node that referenced this issue Aug 27, 2016
jasnell added a commit that referenced this issue Aug 29, 2016
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Sep 8, 2016
Fishrock123 pushed a commit that referenced this issue Sep 9, 2016
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@tniessen
Copy link
Member

tniessen commented Apr 2, 2017

I think we are talking about two unrelated problems. @francisl suggests that the result of extname() and parse() is incorrect for directories, but I do not think that is true.

Currently path.extname and path.parse handle . in directory name as a file extension.

No modern file system relies on file extensions (let's not talk about 8.3) and all major operating systems support (almost) arbitrary combinations of . and other characters. File name extensions have become a rather abstract concept and I think it is reasonable not to put any meaning into extensions when parsing paths.

I cannot think of any problems related to this behavior, maybe close this issue?

@jasnell I think it is a little awkward to break with traditional path resolution, but I see you already pulled that. See man 3 basename:

Trailing '/' characters are not counted as part of the pathname.

@bnoordhuis
Copy link
Member

I agree. The path.extname('/Users/Bob.Dev') example cannot know if Bob.Dev is a directory or a file because it doesn't (and shouldn't) query the file system, it only operates on the string.

The fact that trailing slashes are ignored might be surprising to some but it's consistent with how basename(1) and basename(3) work. It should probably be documented but that's all, pull requests welcome. I'll go ahead and close this out.

tniessen added a commit to tniessen/node that referenced this issue Apr 3, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

Refs: nodejs#6229
benjamingr pushed a commit that referenced this issue Apr 14, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
evanlucas pushed a commit that referenced this issue May 1, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue May 15, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: #12181
Fixes: #6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

PR-URL: nodejs/node#12181
Fixes: nodejs/node#6229
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 26, 2019
This test precedented the official documentation that states that
this is an expected behavior.

Refs: nodejs#6229
Refs: nodejs#12181
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 29, 2019
This test precedented the official documentation that states that
this is an expected behavior.

PR-URL: nodejs#26913
Refs: nodejs#6229
Refs: nodejs#12181
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
This test precedented the official documentation that states that
this is an expected behavior.

PR-URL: #26913
Refs: #6229
Refs: #12181
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 8, 2019
This test precedented the official documentation that states that
this is an expected behavior.

PR-URL: #26913
Refs: #6229
Refs: #12181
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@moshfeu
Copy link

moshfeu commented Jan 6, 2024

For future readers, if the path can only be folder, we can use path.basename

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants