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: make format() consistent and more functional #2408

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@nwoltman
Copy link
Contributor

commented Aug 17, 2015

This PR makes the win32 and posix versions of path.format() consistent in when they add a directory separator between the dir and base parts of the path (always add it unless the dir part is the same as the root part). This fixes the following inconsistencies:

path. Before After
win32.format({dir: 'folder\\', base: 'file.txt'}) 'folder\\file.txt' 'folder\\\\file.txt'
posix.format({dir: 'folder/', base: 'file.txt'}) 'folder//file.txt' (no change)
win32.format({root: 'C:\\', dir: 'C:\\'}) 'C:\\' (no change)
win32.format({root: '\\\\unc\\path\\', dir: '\\\\unc\\path\\'}) '\\\\unc\\path\\' (no change)
posix.format({root: '/', dir: '/'}) '//' '/'

This fixes bugs in path.format() and path.parse() being mirrors of each other (now they truly are mirrors for both win32 and posix).

Also, path.format() is now more functional in that it uses the name and ext parts of the path if the base part is left out, and it uses the root part if the dir part is left out. I added an example to the docs to show the new functionality.

I also removed the check for pathObject.root to be a string, partially because before this commit, pathObject.root wasn't being used for anything in the path.format() function, and also because if that part gets checked then all of the parts should get checked.

Alternatively, the code could check that all of the path parts in the pathObject are strings, in which case it could just go back to using only the dir and base parts (essentially expecting the input to only come from path.parse()).

The discussion for this originated here.

@Trott Trott added the path label Aug 17, 2015

@rvagg rvagg force-pushed the nodejs:master branch from 11c25c2 to ba02bd0 Sep 6, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2015

Sorry for the delay. Starting this off with a CI: https://ci.nodejs.org/job/node-test-pull-request/257/

@nwoltman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2015

Tests look good.
This is a semver-major change so should anyone else weigh in on this?

@nwoltman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2015

Bump

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

@nodejs/ctc ... any thoughts on this one?

@jasnell jasnell added the stalled label Nov 16, 2015

@nwoltman nwoltman force-pushed the nwoltman:path-parse branch Nov 17, 2015

@nwoltman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2015

Resolved conflicts

@rvagg

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

this seems reasonable to me, would be good to get @nodejs/platform-windows involved

@joaocgreis

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

@jasnell

View changes

test/parallel/test-path-parse-format.js Outdated
@@ -5,11 +5,12 @@ var path = require('path');

var winPaths = [

This comment has been minimized.

Copy link
@jasnell

jasnell Nov 19, 2015

Member

Since we're in here making changes anyway, the require decls and these top level decls could be change to const now

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

One comment, otherwise LGTM

path: make format() consistent and more functional
Make the win32 and posix versions of path.format() consistent in when
they add a directory separator between the dir and base parts of the
path (always add it unless the dir part is the same as the root).

Also, path.format() is now more functional in that it uses the name
and ext parts of the path if the base part is left out and it uses
the root part if the dir part is left out.

@nwoltman nwoltman force-pushed the nwoltman:path-parse branch to eb006f3 Nov 20, 2015

@nwoltman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2015

Changed top level vars in test file to const 👍

joaocgreis added a commit to JaneaSystems/node that referenced this pull request Nov 27, 2015

path: make format() consistent and more functional
Make the win32 and posix versions of path.format() consistent in when
they add a directory separator between the dir and base parts of the
path (always add it unless the dir part is the same as the root).

Also, path.format() is now more functional in that it uses the name
and ext parts of the path if the base part is left out and it uses
the root part if the dir part is left out.

Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#2408
@joaocgreis

This comment has been minimized.

Copy link
Member

commented Nov 27, 2015

Landed in d1000b4 .

@woollybogger thank you for the code and for your patience!

@joaocgreis joaocgreis closed this Nov 27, 2015

@nwoltman nwoltman deleted the nwoltman:path-parse branch Dec 13, 2015

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

path: make format() consistent and more functional
Make the win32 and posix versions of path.format() consistent in when
they add a directory separator between the dir and base parts of the
path (always add it unless the dir part is the same as the root).

Also, path.format() is now more functional in that it uses the name
and ext parts of the path if the base part is left out and it uses
the root part if the dir part is left out.

Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#2408
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.