This repository has been archived by the owner on Jul 6, 2019. It is now read-only.
fix(node-path): conform to NPM behavior when dealing with paths that include path.delimiter
#208
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…include path.delimiter
Xunnamius
changed the title
path.delimiterpath.delimiter
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #207
This makes NPX gracefully handle
path.delimiterin a directory name similar to NPM in that they both "just work".I tried to make this PR "immediately" mergeable. Specifically, the new code at line 42 keeps the old NPX behavior unless
path.delimiteris found innpm bin. Otherwise, this PR would be blocked until accompanying changes in this node-which PR are (maybe) accepted. Further, the change on line 183 does not conflict with the current node-which module's behavior as it already accepts an opts object as a second argument. So, even without the accompanying PR from node-which present, NPX maintains its current behavior.It might also be possible to get this working without a node-which PR by using
opts.path, though, after looking over thenode-whichsource, I didn't want to chance any side effects on Windows.There is a slight reduction in
% Branchand% Linecoverage in the tests, though functionality has not changed. If that's a problem, I can write an extra test to cover the new conditional path, but I'd need some help since I'm not at all familiar with writing tap tests :)Let me know if there's anything I can do/missed, or if node-which's
opts.pathwould be a better solution. Thanks for this awesome software!EDIT: looks like the Travis tests are failing for some reason external to this PR...