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

Use OS dependant function to get relative path #1596

Merged
merged 2 commits into from
Nov 18, 2023
Merged

Conversation

sebjulliand
Copy link
Collaborator

Changes

Fixes #1586

On Windows, there is sometimes a case difference in the path's drive letter. For example:
image

In my case, I could reproduce this by having two workspace folders in my workspace folders in my workspace and running a local action after opening a file from the Search results (like described in #1586).

This PR simply checks the target platform and run the path.win32.relative function to get the relative path for local actions if running on Windows.

Checklist

  • have tested my change

@worksofliam
Copy link
Contributor

@sebjulliand I can pull my Windows machine up tomorrow and give this a try.

@worksofliam
Copy link
Contributor

@sebjulliand Works on my Windows machine, but just a though:

Before, we were using path.posix.join to create a posix style path, when actually we needed a Windows style path - thus your PR.

But I don't think you even need to check if it's Windows to use path.win32.join - can't you just use path.join and let the runtime figure out the correct path style?

worksofliam
worksofliam previously approved these changes Nov 6, 2023
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Works but see my comment!

Signed-off-by: Seb Julliand <[email protected]>
@sebjulliand
Copy link
Collaborator Author

Works but see my comment!

You're absolutely right! It is simpler to let path functions handle the OS specifics. See my changes, it simplified it all (while still working 😅).

@worksofliam worksofliam added this to the 2.5.0 release milestone Nov 17, 2023
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

I haven't been able to test on my Windows machine (I have broken it.. long story), but it does continue to work on my macbook.

I trust that if you feel it works on Windows, and I know it works on Mac, I think a merge would be ok.

@sebjulliand
Copy link
Collaborator Author

A merge it is then!

@sebjulliand sebjulliand merged commit 289826e into master Nov 18, 2023
1 check passed
@sebjulliand sebjulliand deleted the fix/windowsFullPath branch November 18, 2023 21:00
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.

Incorrect PATH variables when opening source file from search across files view
2 participants