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.resolve ignores zero-length strings #5928

Closed
wants to merge 2 commits into from
Closed

doc: path.resolve ignores zero-length strings #5928

wants to merge 2 commits into from

Conversation

igorklopov
Copy link
Contributor

path.resolve does not use cwd for zero-length strings as written in docs. Instead it just skips them when iterating.
https://github.com/nodejs/node/blob/master/lib/path.js#L187
https://github.com/nodejs/node/blob/master/lib/path.js#L1189

@mscdex mscdex added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Mar 27, 2016
@benjamingr
Copy link
Member

I get cwd for zero length strings. What you're seeing is what happens when you have a path.resolve with some zero length strings in it, the doc comment is about:

path.resolve("");

See in https://github.com/nodejs/node/blob/master/lib/path.js#L1183 , it sets it to the cwd before that continue.

Thanks for coming by and wanting to make a contribution to the docs :) This one is by design though.

@benjamingr
Copy link
Member

Maybe it would be appropriate to change the text to "are all zero length strings" or something similar

@igorklopov
Copy link
Contributor Author

In the beginning of resolve article one already reads "If after using all from paths still no absolute path is found, the current working directory is used as well". So, cwd is already mentioned. No need to say it twice. Especially for wierd case path.resolve('', '', ''). Recently there was a sentence about non-string arguments. 08085c4 . I think we should add a similar one about empty strings.

@benjamingr
Copy link
Member

I'm +0 on this - not sure it helps clarify too much, but LGTM.

I want to know what other docs members think about these sort of changes @nodejs/documentation

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM though I am also not sure if clarifies it more, but largely because of the API itself. Maybe we could have an issue to rewrite this whole doc part.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 18, 2016
https://github.com/nodejs/node/blob/master/lib/path.js#L187
https://github.com/nodejs/node/blob/master/lib/path.js#L1189

PR-URL: #5928
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Landed in 4c234df

@MylesBorins
Copy link
Contributor

It looks like this change is not applicable to v5.x or v4.x

There was originally copy that mentioned this that was removed in an edit

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
https://github.com/nodejs/node/blob/master/lib/path.js#L187
https://github.com/nodejs/node/blob/master/lib/path.js#L1189

PR-URL: nodejs#5928
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
https://github.com/nodejs/node/blob/master/lib/path.js#L187
https://github.com/nodejs/node/blob/master/lib/path.js#L1189

PR-URL: #5928
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.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.

6 participants