Should this module always return a path ending in a separator?

[phated]

This module was adopted in glob-stream as a drop-in replacement to glob2base; however, we've needed to add a bunch of workarounds into the glob-stream codebase to make this work.

Do you think it makes sense to always return a path ending in a separator due to path.dirname being called on each segment (thus the parent should always be a directory)?

I'd like to write:

var basePath = toAbsoluteGlob(globParent(myGlob), opts);

but currently have to use:

var basePath = toAbsoluteGlob(globParent(myGlob) + '/', opts);

to get my test suite to pass correctly.

@jonschlinkert @es128 thoughts?

[jonschlinkert]

could this be because of something that to-absolute-glob is doing? (or could be doing?)

[jonschlinkert]

also, there is an existing related issue where we discussed this at one point (I believe it was related): #3

[phated]

@jonschlinkert I don't think to-absolute-glob could/should be handling this. Your open PR over there already re-adds a trailing slash if it existed; however, it doesn't exist because, in my usage, glob-parent always removes the trailing separator due to path.dirname usage internally. This was opened more of a question of semantics since we need that trailing slash in our usage.

[tunnckoCore]

I think it make sense what @phated says. Or at least for 90% (or more) of the cases it is used for such thing.

[es128]

to get my test suite to pass correctly.

Is this just to preserve the pre-existing test assertions that assumed the output would contain the trailing separator, or is there some further need for it that I'm not seeing? It strikes me as a little arbitrary, proper use of the core path lib like path.join() would not necessitate this.

That said, we've made changes to bring this in line with glob2base behavior before and I don't mind doing it again. Seems like it would be better to put it out with a major version bump than to go evaluate whether this might have any adverse impact on any downstream users.

Would it just be return str + path.sep; ?

[phated]

@es128 I was mostly concerned about an issue we received back in August (gulpjs/glob-stream#68) in which the basePath didn't end in a trailing slash and a user claimed it broke plugins on Windows.

The more I think about this, we are already removing the trailing separator on file.base in version 2.0 of vinyl (https://github.com/gulpjs/vinyl/blob/master/index.js#L214) so maybe glob-stream should just remove the trailing slash behavior altogether. Thoughts?

[es128]

This strikes me as one of those things that's ripe for bikeshedding, and for which consistency is more important than the one choice or the other. I could argue either side and ultimately find myself agnostic on it overall.

What would be the best way to achieve consistency among the modules within our control with the least amount of change/disruption?

[phated]

I am pushing forward on the next breaking major release on glob-stream so I think the breaking change would belong there (to remove the trailing slash). I've asked the author of the linked issue for any more details on broken plugins so I can review them. I'll follow up here once I have any other information. I think this should probably remain open while I investigate further.

[es128]

Sure, I'm on board with that and will be supportive of whatever you end up concluding would be best.

[es128]

Closing per discussion in #7.

@phated if I'm jumping the gun here let me know. I'm fine with reopening if it wasn't a final decision

[phated]

I'm fine with closing this. I like the consistency of removing the trailing separator (this decision bubbles up through our dependency tree already).