Search PATHEXT instead of 3 hardcoded values#290
Conversation
src/which.js
Outdated
|
@isiahmeadows could you |
src/which.js
Outdated
There was a problem hiding this comment.
Might be good to mentioned that WinXP lacks this thing in the comment. Also not sure if supporting Windows XP is a good idea. It is quite old and unsupported even by Microsoft.
There was a problem hiding this comment.
I don't see much harm in the default value. I agree that WinXP doesn't need to be supported by us, since the OS is end-of-life by Microsoft. However, just in case it's perhaps possible to delete environmental variables in Windows (is this even possible?), It'd be good to keep some fall-back value.
There was a problem hiding this comment.
You can delete environmental variables but if you delete this one I'm guessing shelljs would be the least of your problems :D
There was a problem hiding this comment.
Still, a default value is a good fallback so we should keep it.
There was a problem hiding this comment.
^ Good point. But yes, I also vote we keep it for no other reason than extra safety measure.
There was a problem hiding this comment.
I'm clarifying the comment as it's a precautionary measure.
|
LGTM FYI: Windows also has a built-in |
src/which.js
Outdated
There was a problem hiding this comment.
Totally optional suggestion
Perhaps we don't need this much info in the README? The README is supposed to give a brief overview of commands, and this might be too much detail. I think it's sufficient to say something to the effect of "On Windows, this uses the PATHEXT environment variable to append the extension" here.
I think it's better to have this comment down below in the function body.
|
Do note that I use Linux, my Windows installation is rather broken ATM, and my computer is too old/underpowered to run a VM, so I don't know how well this runs, and I kinda feel guilty not being able to test this. 😦 But feel free to test this yourself. I did add a test. It's also a trivial enough patch that it should work. (@nfischer If you all got Appveyor to run for PRs as well, that'd be awesome, probably even for you, considering this supports Windows. 😉) I submitted the original PR, #134, about 1 1/2 years ago, when my Windows installation actually worked (mine needs way more troubleshooting than I have time for ATM - my standard user directory requires Admin privileges to even read). |
|
I'll test this out on my Windows machine today and see how it runs. Thanks again! |
test/which.js
Outdated
There was a problem hiding this comment.
Oops...will fix when I get home...
|
The nits are all totally optional. They're just for stylistic consistency (which we aren't very good at anyway, as you can probably see by other lines in this test that differ from the style I'm suggesting). The big issue is the comment about case-insensitive comparison (and also see the typo I pointed out). Once that gets updated, I'll test again and give the thumbs up. |
|
@isiahmeadows Could you rebase this off master? We now have Windows CI set up with appveyor, and it'd be great to verify this under that (although I'm fairly certain it'll pass after the rebase). |
|
I like that GitHub has this "Update Branch" button where I could do that here. 😄 |
|
What? They added such a button‽‽‽ Thanks! Ari — |
|
That's what I just used. And it doesn't appear to be passing... I'll fix it when I get home. I've been a bit busy lately. |
|
This latest push won't fix the build, but it'll give me a better idea what happened. |
|
@nfischer It's surprising that the AppVeyor build isn't marked as required...I'm guessing that was an oversight? (I just noticed this.) |
Redo of shelljs#134 `which` now searches through PATHEXT on Windows, and it also now does a case-insensitive comparison. This better fits the Windows environment, where the OS usually ignores case.
|
I've rebased against |
|
@ariporad I think this is a recent addition, where if you don't have merge conflicts, you can just update your remote branch through GitHub itself instead of locally having to do it. It's just like with merging branches. |
|
Edit: just checked the code (and meant to hit cancel on this comment, not "close and comment". Looks like checkpath() works as expected. LGTM |
|
LGTM (in case lgtm.co missed it) |
feat(Windows): search PATHEXT instead of 3 hardcoded values
|
Thanks @isiahmeadows! And yeah that was an oversight, At the time we added it, our windows build was broken. |
|
@ariporad Okay. |
Redo of #134.
Since it was only a few lines, it was easy to rewrite against master.