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

Feature request: Add path.split() as a counterpart to path.join() #5523

Closed
callumlocke opened this issue Mar 2, 2016 · 20 comments
Closed

Feature request: Add path.split() as a counterpart to path.join() #5523

callumlocke opened this issue Mar 2, 2016 · 20 comments
Labels
feature request Issues that request new features to be added to Node.js. path Issues and PRs related to the path subsystem.

Comments

@callumlocke
Copy link

It's weird that path.join('foo', 'bar') exists while for the reverse you have to use 'foo/bar'.split(path.sep).

Can we implement path.split('foo/bar') for consistency? I can PR if agreed.

NB. there was an issue years ago titled "path.split() needed" but the discussion morphed into one about getting path.parse() implemented, and path.split() never happened.

@Trott Trott added feature request Issues that request new features to be added to Node.js. path Issues and PRs related to the path subsystem. labels Mar 2, 2016
@alexjeffburke
Copy link
Contributor

I wonder if a slight wrinkle is path.sep is platform specific - presumably to deal with that a split() function would first call path.normalize(). The side effects of conversion might need some thinking through.

@zeusdeux
Copy link
Contributor

zeusdeux commented Mar 7, 2016

@alexjeffburke what side effects are you talking about?

@alexjeffburke
Copy link
Contributor

Given path.sep is platform specific and my guess is the internal normalise would be required, it was just whether 'quietly' returning such a path to the caller which they may then go on and do other things with could result in any behavioural weirdness. Perhaps that falls on the side of a documentation concern.

@callumlocke
Copy link
Author

Does this basically cover it:

path.split = function (dir) {
  return dir.split(path.sep);
}

Or is there more to it? (I know the path functions tend to do a bunch of type-checking and assertions, but anything else?)

@evanlucas
Copy link
Contributor

That won't work for the following example:

path.win32.split('/a/b/c')

There is definitely more to take into account. I really don't think this is something that should be in core. We already have path.sep and there are a lot of little things that could be unexpected.

@callumlocke
Copy link
Author

Do you mean the need to make different versions for the two styles?

path.win32.split = function (dir) {
  return dir.split(path.win32.sep);
}

path.posix.split = function (dir) {
  return dir.split(path.posix.sep);
}

I'm sure there are various other edge cases but can't we work them out here?

there are a lot of little things that could be unexpected

Then rather than leaving it to all all users to think about and solve these things themselves, isn't it worth us identifying all the gotchas, devising a pair of solid functions with well defined behaviour that addresses them, and adding them to core?

@wacky6
Copy link

wacky6 commented Mar 24, 2016

Maybe restrict the input to strings returned by path.* functions?

I don't think making different versions is a good idea, they require additional code to ensure portability.

@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

I think I'm leaning towards -1 having this in core. Seems like a great userland module tho.

@justinfagnani
Copy link

I was trying to use path.posix.normalize to convert Windows paths to posix paths, but that didn't work, so I thought I would use path.split() and path.join() to break apart the path and convert it myself, but path.split() doesn't exist. This really seems like something that should be in core, as dir.split(path.sep) won't always do the right thing.

@julianduque
Copy link
Contributor

julianduque commented May 27, 2016

path.split was part of core, it was introduced in v0.3.1 (9996b45) but removed in v0.3.4 (7c731ec)

I'm -1 on adding this back to core

@saghul
Copy link
Member

saghul commented May 27, 2016

FWIW, both Ruby and Python have a split functionality, which is not the opposite as join. It splits a path into [dirname, basename].

I'd be +1 on having that.

@Trott
Copy link
Member

Trott commented Jul 7, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 7, 2017
@justinfagnani
Copy link

I would love to see this re-opened (and eventually addressed, of course).

@Trott
Copy link
Member

Trott commented Jul 21, 2017

I would love to see this re-opened (and eventually addressed, of course).

Implement it as a userland module. If the module is rock-solid and lots of people use it, you can make the case that perhaps it belongs in core. (And even if that case is ultimately rejected, you'll have done a service for the community and the ecosystem.)

@justinfagnani
Copy link

Well, you asked if anyone wanted it open...

@Trott
Copy link
Member

Trott commented Jul 21, 2017

Well, you asked if anyone wanted it open...

That's a more-than-fair point. 😆

@rainabba
Copy link
Contributor

rainabba commented May 15, 2018

Also here while searching for a path.split() function that would return an array of the path elements. I'd like to think a simple string.split( path.sep ) would work, but I've been using filesystems since DOS 3.0 and know how absurdly messy and unpredictable they can be in reality and I'm going to be dealing in dynamic names/paths so I want to assume as little as possible.

Anyone have a library/function they've implemented and come to trust?

Presently looking at the logic here: https://www.safaribooksonline.com/library/view/python-cookbook/0596001673/ch04s16.html

@TomasHubelbauer
Copy link

TomasHubelbauer commented Aug 24, 2018

That's a more-than-fair point. 😆

Should this stay open then?

@Trott
Copy link
Member

Trott commented Aug 24, 2018

Should this stay open then?

I don't think so at this time. 5 core developers have weighed in on this issue. All 5 are opposed to the idea of adding path.split() to core (except @saghul who is in favor of path.split() but not as apparently envisioned in this feature request). I think the proper state for a feature request that is being respectfully declined is for it to be closed.

If you only count four: I'm including myself as the fifth. I didn't have strong feelings about it when I closed it the first time, but I've developed some stronger opinions about what does and doesn't belong in core in the last year.

I will play devil's advocate though and lay out some possibilities for moving this forward:

  • Some winds have changed a bit recently. In particular, @bcoe managed to get a version of mkdirp() landed in master and has expressed interest in something like rimraf() too. Those are in fs and not path, but it seems related. So maybe there are Collaborators who would advocate for this? (I can't think of any, except maybe @bcoe, which is why I'm @-mentioning them.)

  • If someone is really motivated to get this into Node.js and doesn't mind an uphill challenge, they can open a pull request. Just as long as they know that there is a high likelihood that it would get discussed and then not included in Node.js, that's worth a shot.

  • The way mkdirp() happened above was that there was a rock-solid userland implementation that was so ubiquitously used, that it made it clear the feature made sense for Node.js core from a "batteries included" perspective. If there's an npm module out there that does this and it is getting millions of downloads and it's not a pile of spaghetti code that needs bug-fixes every 10 minutes, that could be persuasive.

@TomasHubelbauer
Copy link

TomasHubelbauer commented Aug 25, 2018

There is https://www.npmjs.com/package/filepath#split, which doesn't exactly get millions of downloads and does what everyone is already doing anyway: split by any slash.

I feel like there's more nuance to it than that, which people are likely to get wrong in their own implementations, that's why I feel like it should be a core feature, but I respect that the actual would-be-maintainers feel differently.

So, for anyone landing on this issue from Google, feel free to use and improve the above package (the more rock solid it is, the more likely it is to get accepted to core, see above) or just keep on regexing knowing that that's the state of the art out there so if you just need path.split on top of what native path already does, there's no harm in just slapping that regex on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

No branches or pull requests