Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path environment variable tweaks #572

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

niik
Copy link
Member

@niik niik commented Jun 5, 2024

@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 niik requested review from sergiou87 and tidy-dev June 5, 2024 11:00
Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😗👌

@niik
Copy link
Member Author

niik commented Jun 5, 2024

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.

@niik niik merged commit 03fa09d into fix-buiding-env-path Jun 5, 2024
7 checks passed
@niik niik deleted the with-paths-like-this-who-needs-enemies branch June 5, 2024 11:24
Copy link

@Jenn-88 Jenn-88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

payloads??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants