replace the colon with the OS delimiter

benoror / better-npm-run

🏃‍♂️ Better NPM scripts runner

MIT License

639 stars 56 forks source link

replace the colon with the OS delimiter #40

Closed memorydump closed 8 years ago

memorydump commented 8 years ago

Change Summary

Replace the ':' the delimiter separator with the OS delimiter

benoror commented 8 years ago

Hi @memorydump ! Thx for your PR, can you explain further the reasoning behind this?

carnesen commented 8 years ago

It would be bad to do this replacement for every script.env value. Suppose, for example, I pass in a database connection string (e.g. "mongo://localhost:20170/foo"). On Windows that'd get hosed by the replacement : --> ; If anything the replacement should be restricted to PATH if that's passed in but that would be bad on Windows too because if I set path correctly, e.g. "C:\mongodb\bin;C:\whatever\bin", the colons would get turned into semi's which would be bad.

memorydump commented 8 years ago

Basically, it is for getting the env PATH variable working in windows, since windows use a "semicolon" for the delimiter as opposed to a "colon". But @carnesen brought good points. I am gonna close the PR and create a new one with a better solution.