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: Adding note about empty strings in path module #2106

Closed
wants to merge 2 commits into from

Conversation

thefourtheye
Copy link
Contributor

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.

@mscdex mscdex added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Jul 5, 2015
@Trott
Copy link
Member

Trott commented Jul 6, 2015

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.

path.isAbsolute('.') // false

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

@brendanashworth
Copy link
Contributor

@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
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the behaviour varies on win32 and posix.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Bump!

@Trott
Copy link
Member

Trott commented Jul 12, 2015

LGTM.

@Trott
Copy link
Member

Trott commented Jul 12, 2015

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
Copy link
Contributor Author

Thanks @Trott :-)

@Trott
Copy link
Member

Trott commented Jul 14, 2015

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

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 14, 2015
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
Copy link
Member

Trott commented Jul 14, 2015

merged in 65963ec

@Trott Trott closed this Jul 14, 2015
@thefourtheye thefourtheye deleted the path-doc-fix branch July 14, 2015 16:22
@thefourtheye
Copy link
Contributor Author

@Trott Thanks :-)

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.

5 participants