path.split implementation #2766

Closed
wants to merge 2 commits into
from

Projects

None yet

9 participants

@gero3
gero3 commented Feb 16, 2012

I added an implementation to create a split method in the path module. This is created based on #1224.

I ask for input
@isaacs @bnoordhuis

@isaacs
isaacs commented Feb 18, 2012

I like the basic idea here. However, I think that it needs a bit more refinement.

  1. It should be called 'path.parse' rather than 'path.split', since that's what it's doing. path.split would just be p.split(/\/+/) on unix, and p.split(/[\/\\]+/) on windows. This is turning it into an object, which is very useful, but different.
  2. If there's a path.parse, then there should be a symmetrical path.format that turns the object into a string.
  3. Please make sure that the code lints properly. There's a lot of extra spaces lining things up: don't do that.
  4. The unix result is kind of weird. There's really no "device" in the path on unix, so that should probably just be left out.
  5. It needs tests (a lot, especially really weird chars etc.) and documentation.

@igorzi, @piscisaureus Do you feel that this captures the appropriate information about a windows path?

@Nodejs-Jenkins

Can one of the admins verify this patch?

@ronkorving

What's the status on this? (poke @gero3 )

@isaacs
isaacs commented Jul 19, 2013

@ronkorving @gero3 It's still waiting for someone to get it over the finishline.

Requirements:

  1. Add path.format which does the inverse of path.parse.
  2. A buttload of tests.
  3. Rebase onto current master.
  4. make jslintfix to fix the style issues.
@gero3
gero3 commented Jul 19, 2013

I'll have another look at it tonight

@jquense
jquense commented Dec 7, 2013

I'd take a stab at implementing this since @gero3 seems to be out of the game. Tho I am not sure what the "device" for UNC paths should be. \mycomp\a\path\to\file.js is going to be parsed oddly in this commit ("\mycomp\a"), should the device be "\mycomp" or ""?

@indutny
Member
indutny commented Mar 18, 2014

ping ;)

@litmit
litmit commented Mar 18, 2014

See also #6976

@Mithgol
Mithgol commented Jul 24, 2014

Pull request #7429 might fix this issue.

@JacksonTian

anyone can close it?

@indutny indutny closed this Aug 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment