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

Remove trailing slash from path element #7209

Closed
wants to merge 1 commit into from

Conversation

magnushiie
Copy link

@magnushiie magnushiie commented Apr 19, 2019

Summary

Every character in Windows environment is a precious resource, should not append an unnecessary slash at the end.

Furthermore, it's inconsistent with the missing trailing slash in the AppData path a few lines below.

Every character in Windows environment is a precious resource, should not append an unnecessary slash at the end.
@magnushiie
Copy link
Author

Note, this reverts a recent change by @derekellison #5387 - that PR mentions that the trailing slash is "as per windows path requirements", there is no requirement for a trailing slash in PATH elements - most of the Windows system paths do not have a trailing slash.

@arcanis
Copy link
Member

arcanis commented Apr 19, 2019

Ping @derekellison - I don't use Windows so I can hardly vouch for any approach here. #5387 mentions it fixes a bug, can you detail which one and how that's a fix?

@magnushiie
Copy link
Author

I've been using a build with this change for over 2 weeks and it works fine. Maybe the bug was just referring to the opinion that path elements should have a trailing slash.

I would like to get this PR off my pending PR list, can we merge this?

@magnushiie
Copy link
Author

@arcanis it seems @derekellison is not here to respond - can we merge this? I think #5387 wasn't a bug fix and was a mistake.

@derekellison
Copy link
Contributor

This was a bug in windows a few years ago - not sure if its still an issue. See this for background of the issue: #1648

@magnushiie
Copy link
Author

I believe these comments do not support the conclusion that adding a backslash fixed it - it was probably rather the manual updating the environment variables via the UI that triggered the refresh. I didn't see there any evidence of a process having a PATH correctly set up (without the trailing backslash) and not being able to find yarn.

@magnushiie magnushiie closed this Jun 17, 2024
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.

4 participants