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

fix(util): Path.parent now works on windows (#1168) #1180

Merged
merged 2 commits into from
May 24, 2024

Conversation

xudyang1
Copy link
Contributor

On Windows, both forward slash / and backslash \\ work as the path separator. Linux and MacOS can have backslash as a valid filename character.

Unit tests are also provided for each platform because Path.parent depends on the local variable sep which depends on jit.os.

Fixes #1168

@L3MON4D3
Copy link
Owner

Hey, thanks for tackling that :)

This looks good (differentiating on definition-level, not at runtime is cool :D), and I'd like to propose two improvements/changes:

  1. parent should retain the previous behaviour of removing the trailing sep from the path, that fits with the remainder of the current code (join always appends sep, for example)
  2. Regarding testing: I like that you added tests for both platforms, but since these tests can be run on either platform, it would be cool to also do this. For example, you could (in the definition of parent) allow some global variable that is unlikely to be set during normal operations, maybe ___LUASNIP_TEST_SEP_OVERRIDE, to override what we detect via sep, and then set that to / or \ depending on which test is currently running

@xudyang1
Copy link
Contributor Author

Hey, thanks for tackling that :)

This looks good (differentiating on definition-level, not at runtime is cool :D), and I'd like to propose two improvements/changes:

  1. parent should retain the previous behaviour of removing the trailing sep from the path, that fits with the remainder of the current code (join always appends sep, for example)
  2. Regarding testing: I like that you added tests for both platforms, but since these tests can be run on either platform, it would be cool to also do this. For example, you could (in the definition of parent) allow some global variable that is unlikely to be set during normal operations, maybe ___LUASNIP_TEST_SEP_OVERRIDE, to override what we detect via sep, and then set that to / or \ depending on which test is currently running

If I understand that correctly:

  1. the previous behavior of parent returns parent directory without trailing sep for files only, but for directories, for example /home/user/, it returns nil but not /home. To retain the previous behavior, require("luasnip.util.path").parent("/somefile.txt") should return "" (empty string). Although it looks weird, this is a valid parent because empty string in lua is a truthy value. I will also add some docs to the parent function to clarity the above behaviors.
  2. Instead of making tests platform dependent, we should make them path separator dependent so that the tests can even run on other platforms that use either forward slash or backslash as path separator.

@xudyang1 xudyang1 force-pushed the fix/path_parent branch 8 times, most recently from 91a96e3 to 39766a2 Compare May 22, 2024 20:32
On Windows, both forward slash `/` and backslash `\\` work as the path
separator. Linux and MacOS can have backslash as a valid filename
character.

Unit tests are also provided for each platform because `Path.parent`
depends on the local variable `sep` which depends on `jit.os`.

Fixes L3MON4D3#1168
@L3MON4D3
Copy link
Owner

Nice, this looks great!
Thanks for adding the disclaimer on C: for windows, I doubt there will ever be a case where a snippet-repo lives in a root-directory, but this should find it :) (and if that ever happens, maybe we can just switch over to the fs-module built into neovim :) )

Thank you!!

@L3MON4D3 L3MON4D3 merged commit d890d6d into L3MON4D3:master May 24, 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.

Windows: friendly snippets not loaded
2 participants