search

desktop / dugite

Elegant bindings for working with Git in your Node applications
MIT License
462 stars 149 forks source link

Path environment variable tweaks #572

Closed niik closed 5 months ago

niik commented 5 months ago

@tidy-dev @sergiou87 Opening this PR with some proposed changes to #570.

With this I've ensured that the test added by @sergiou87 fails not just on CI but on any environment regardless of their casing of the path environment variable.

Additionally I've extended the merging logic to remove any other casing of the path environment variable (not just Path) and ensure that it's always set in uppercase.

Having spent the morning thinking about this problem I do think that we should come up with a more comprehensive solution for Windows where we treat all environment variables as case-insensitive but case-preserving.

niik commented 5 months ago

I do like the direct filtering out of path instead of adding twice and then deleting. Tho I do find this harder to digest with the Object.x functions.

Agreed, I hoped that the comments would offset that some but I'll have a PR in a bit here which takes this problem on more holistically and hopefully that'll make it easier to consume again.