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

doc: path functions ignore trailing slashes #12181

Closed
wants to merge 4 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Apr 3, 2017

Add notes about path.parse(), path.basename() and path.dirname() ignoring trailing slashes as suggested by @bnoordhuis at #6229.

Refs: #6229

Checklist
Affected core subsystem(s)

doc

@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 3, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I'd simply say "Trailing '/' characters are ignored." and leave it at that. Not everyone will be familiar with POSIX and "not counted as part of the pathname" could lead a reader to believe it's counted as part of something else; it's better to be explicit about it being ignored.

Add notes about path.parse(), path.basename() and path.dirname()
ignoring trailing slashes.

Refs: nodejs#6229
@tniessen
Copy link
Member Author

tniessen commented Apr 3, 2017

@bnoordhuis You are probably right. By the way, should I add a note about Windows? Something along "Trailing '/' (and '\' on Windows) characters are ignored."?

path.posix.basename('/home/foo\\') == 'foo\\'
path.win32.basename('/home/foo\\') == 'foo'

@bnoordhuis
Copy link
Member

Yes, a note on Windows paths would be good.

@tniessen
Copy link
Member Author

tniessen commented Apr 3, 2017

@bnoordhuis Done :)

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.

It might be best to just say "trailing slashes are ignored"

For example, path.posix.dirname('/foo/bar\\') drops the backslashes too.

@sam-github
Copy link
Contributor

sam-github commented Apr 3, 2017

path.posix.dirname doesn't drop the backslashes, it considers them part of the basename:

> path.posix.dirname('/foo/bar\\') 
'/foo'
> path.posix.basename('/foo/bar\\') 
'bar\\'

Unlike Windows, which does drop them:

> path.win32.dirname('/foo/bar\\')
'/foo'
> path.win32.basename('/foo/bar\\')
'bar'
> path.win32.basename('/foo/bar\\//\\//')
'bar'

@cjihrig
Copy link
Contributor

cjihrig commented Apr 3, 2017

Ah, thanks.

doc/api/path.md Outdated
@@ -68,7 +68,8 @@ changes:
* Returns: {string}

The `path.basename()` methods returns the last portion of a `path`, similar to
the Unix `basename` command.
the Unix `basename` command. Trailing `/` (and `\\` on Windows) characters are
ignored.
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 the wording is too subtle, its not clear enough that windows supports two dir seps, and will strip both. I would say "Trailing / characters are ignore, and on Windows trailing / and \ characters are both ignored". Or perhaps "trailing directory seperators are ignored, see REF", and explain that unix has only one directory seperator, /, but that win32 supports both / and \ (or a mixture of the two) as directory seps.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about moving the paragraph

Note: On Windows, both the forward slash (/) and backward slash (\\) characters are accepted as path delimiters; however, only the backward slash (\\) will be used in return values.

from the very bottom to the section about path.sep and referencing that section: "Trailing directory separators are ignored, see [path.sep][]"?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. but "used in return values" means what?

> path.win32.dirname('\foo/that/bar\\//\\//')
'\foo/that'

^--- clearly / and \ are both used in return values

Copy link
Member Author

Choose a reason for hiding this comment

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

This is preexisting (I copied that paragraph from path.md about path.win32). Is it to be considered a bug in dirname or is it a documentation inaccuracy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a doc inaccuracy to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed that "path delimiters" is not the best wording as well, considering how path.delimiter is defined. How about

Note: On Windows, both the forward slash (/) and backward slash (\) are accepted as path segment separators; however, if separators are to be added by the Windows-specific implementations of the path methods, only the backward slash (\) will be used.

@tniessen
Copy link
Member Author

tniessen commented Apr 5, 2017

@bnoordhuis @sam-github I tried to incorporate the changes discussed with Sam, thoughts on this? :)

doc/api/path.md Outdated
*Note*: On Windows, both the forward slash (`/`) and backward slash (`\`) are
accepted as path segment separators; however, if separators are to be added by
Windows-specific implementations of the `path` methods, only the backward slash
(`\`) will be used.
Copy link
Member

Choose a reason for hiding this comment

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

I'd change the part after the semicolon to "the path methods only add backward slashes (\)."

"if ... to be ..." is probably hard to follow for people without a good command of English.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, done.

@tniessen
Copy link
Member Author

@bnoordhuis Would you mind reviewing my changes since your last review? :)

@benjamingr
Copy link
Member

@bnoordhuis can you please confirm the changes performed are to your satisfaction?

@benjamingr
Copy link
Member

Landed in 7fd2923

Thanks for the contribution!

@benjamingr benjamingr closed this Apr 14, 2017
benjamingr pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@MylesBorins
Copy link
Contributor

I've backported to v6.x. If this is inaccurate for that release please let us know

MylesBorins pushed a commit that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request 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 <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request 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 pull request 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 <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit that referenced this pull request 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 <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit that referenced this pull request 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 <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.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. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.