doc: Adding note about empty strings in path module #2106

thefourtheye · 2015-07-05T10:03:36Z

The path module's join, normalize, isAbsolute, relative and resolve
functions return/use the current directory if they are passed zero
length strings.

> process.version
'v2.3.4-pre'
> path.join('')
'.'
> path.win32.join('')
'.'
> path.posix.join('')
'.'
> path.win32.normalize('')
'.'
> path.posix.normalize('')
'.'
> path.win32.isAbsolute('')
false
> path.posix.isAbsolute('')
false
> path.win32.relative('', '')
''
> path.posix.relative('', '')
''
> path.win32.relative('.', '')
''
> path.posix.relative('.', '')
''
> path.posix.resolve('')
'/home/thefourtheye/Desktop'
> path.win32.resolve('')
'\\home\\thefourtheye\\Desktop'

Since empty paths are not valid in any of the operating systems people
normally use, this behaviour might be a surprise to the users. This
commit introduces "Notes" about this, wherever applicable in path's
documentation.

Trott · 2015-07-06T06:15:10Z

nit: I think empty string is clearer than zero length string. Or, at least, zero-length string instead of zero length string.

Nit aside, LGTM.

brendanashworth · 2015-07-06T07:03:40Z

doc/api/path.markdown

    path.isAbsolute('.')         // false

+*Note:* If the path is a zero length string then `false` will be returned.


I'm not sure whether this is really clarifying anything?

@brendanashworth Since it is a special case, I thought it would be better to document that as well. We can remove it, if it doesn't make much sense to keep.

Your call - imo I would throw on this, but that is out of the picture so I'll leave it up to you.

@brendanashworth Well, in other cases empty string means the current directory, and this is an exception. So, I think it would be better to leave it in.

thefourtheye · 2015-07-06T07:10:40Z

@Trott I have read in few places in the Internet that empty string sometimes means a string with only space characters as well. So, I ll better go with zero-length string. Thanks for the suggestion :-)

brendanashworth · 2015-07-06T07:10:55Z

I like where this is going! I feel like the wording of "the path" could be a little bit ambiguous for some of them, like path.resolve though. Thoughts?

thefourtheye · 2015-07-06T07:14:28Z

@brendanashworth Oh yeah, will path string passed as parameter be fine?

brendanashworth · 2015-07-06T07:50:21Z

@thefourtheye that doesn't specify which path string ([from ...], to?). I'm not familiar with the case enough it seems - do all/any of the path strings trigger this case? I think it should specify whether it is from / to if not.

thefourtheye · 2015-07-06T07:53:22Z

@brendanashworth It is applicable to both from and to. How can I better phrase it?

thefourtheye · 2015-07-06T14:11:35Z

@brendanashworth I updated the messages a little bit based on your suggestion. PTAL. cc @Trott

Fishrock123 · 2015-07-06T16:55:35Z

doc/api/path.markdown

    path.isAbsolute('.')         // false

+*Note:* If the path string passed as parameter is a zero-length string,
+        unlike other path module functions, zero-length string will be used
+        as-is and `false` will be returned.


IIRC the behaviour varies on win32 and posix.

If they aren't there already, maybe add tests for the situations covered by these notes and see what happens on CI. Then you will know for sure whether the behavior is universal or platform-dependent. Include comments indicating that the tests are there to cover edge cases noted in the documentation.

@Fishrock123 I don't have a Windows machine, but when I tried in REPL, I got false for both the versions.

@Trott Thanks for the suggestion man :-) I included a simple test now. PTAL.

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/121/

@Trott Thank you :-) Looks like the test is passing in all the environments.

Just manually tested on Windows for good measure, and path.isAbsolute('') returned false. So that seems OK.

The path module's `join, normalize, isAbsolute, relative and resolve` functions return/use the current directory if they are passed zero length strings. > process.version 'v2.3.4-pre' > path.win32.join('') '.' > path.posix.join('') '.' > path.win32.normalize('') '.' > path.posix.normalize('') '.' > path.win32.isAbsolute('') false > path.posix.isAbsolute('') false > path.win32.relative('', '') '' > path.posix.relative('', '') '' > path.win32relative('.', '') '' > path.posix.relative('.', '') '' > path.posix.resolve('') '/home/thefourtheye/Desktop' > path.win32.resolve('') '\\home\\thefourtheye\\Desktop' Since empty paths are not valid in any of the operating systems people normally use, this behaviour might be a surprise to the users. This commit introduces "Notes" about this, wherever applicable in `path`'s documentation.

These testcases are specific to one uncommon behaviour in path module. Few of the functions in path module, treat '' strings as current working directory. This test makes sure that the behaviour is intact between commits. See: nodejs#2106

thefourtheye · 2015-07-11T05:50:09Z

Bump!

Trott · 2015-07-12T02:34:16Z

LGTM.

Trott · 2015-07-12T02:41:08Z

Unless someone beats me to it or raises an objection between now and then, I'll merge this some time in the next 48 to 72 hours (roughly some time on Tuesday PDT).

thefourtheye · 2015-07-12T02:47:16Z

Thanks @Trott :-)

Trott · 2015-07-14T15:54:17Z

Rebased against master and running CI one more time for good measure: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/150/

The path module's `join, normalize, isAbsolute, relative and resolve` functions return/use the current directory if they are passed zero length strings. > process.version 'v2.3.4-pre' > path.win32.join('') '.' > path.posix.join('') '.' > path.win32.normalize('') '.' > path.posix.normalize('') '.' > path.win32.isAbsolute('') false > path.posix.isAbsolute('') false > path.win32.relative('', '') '' > path.posix.relative('', '') '' > path.win32relative('.', '') '' > path.posix.relative('.', '') '' > path.posix.resolve('') '/home/thefourtheye/Desktop' > path.win32.resolve('') '\\home\\thefourtheye\\Desktop' Since empty paths are not valid in any of the operating systems people normally use, this behaviour might be a surprise to the users. This commit introduces "Notes" about this, wherever applicable in `path`'s documentation. The tests makes sure that the behaviour is intact between commits. PR-URL: nodejs#2106 Reviewed-By: Rich Trott <[email protected]>

Trott · 2015-07-14T16:19:09Z

merged in 65963ec

thefourtheye · 2015-07-14T16:22:39Z

@Trott Thanks :-)

thefourtheye mentioned this pull request Jul 5, 2015

path: Throwing error on zero length strings #2105

Closed

mscdex added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Jul 5, 2015

brendanashworth reviewed Jul 6, 2015
View reviewed changes

thefourtheye force-pushed the path-doc-fix branch from 809d8de to 63874da Compare July 6, 2015 07:10

Fishrock123 reviewed Jul 6, 2015
View reviewed changes

thefourtheye added 2 commits July 8, 2015 01:04

thefourtheye force-pushed the path-doc-fix branch from 10b2405 to a65d5fd Compare July 7, 2015 20:00

Trott closed this Jul 14, 2015

thefourtheye deleted the path-doc-fix branch July 14, 2015 16:22

Fishrock123 mentioned this pull request Jul 16, 2015

Release Proposal: v2.4.0 #2189

Merged

eush77 mentioned this pull request Jan 23, 2016

path.relative(dir, dir) returns empty string #4826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Adding note about empty strings in path module #2106

doc: Adding note about empty strings in path module #2106

thefourtheye commented Jul 5, 2015

Trott commented Jul 6, 2015

brendanashworth Jul 6, 2015

thefourtheye Jul 6, 2015

brendanashworth Jul 6, 2015

thefourtheye Jul 6, 2015

thefourtheye commented Jul 6, 2015

brendanashworth commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

brendanashworth commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

Fishrock123 Jul 6, 2015

Trott Jul 6, 2015

thefourtheye Jul 7, 2015

Trott Jul 7, 2015

thefourtheye Jul 7, 2015

Trott Jul 12, 2015

thefourtheye commented Jul 11, 2015

Trott commented Jul 12, 2015

Trott commented Jul 12, 2015

thefourtheye commented Jul 12, 2015

Trott commented Jul 14, 2015

Trott commented Jul 14, 2015

thefourtheye commented Jul 14, 2015

		path.isAbsolute('.') // false

		Note: If the path is a zero length string then `false` will be returned.

doc: Adding note about empty strings in path module #2106

doc: Adding note about empty strings in path module #2106

Conversation

thefourtheye commented Jul 5, 2015

Trott commented Jul 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Jul 6, 2015

brendanashworth commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

brendanashworth commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

thefourtheye commented Jul 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Jul 11, 2015

Trott commented Jul 12, 2015

Trott commented Jul 12, 2015

thefourtheye commented Jul 12, 2015

Trott commented Jul 14, 2015

Trott commented Jul 14, 2015

thefourtheye commented Jul 14, 2015