-
Notifications
You must be signed in to change notification settings - Fork 131
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
Allow case insensitive search for .tmtheme
paths
#467
Conversation
797fed1
to
5b7a5bc
Compare
5b7a5bc
to
b438dc9
Compare
Not at all sure why CI would fail here. |
Is this project still maintained? |
Several people are watching project activity as far I know. The main reason no action has been taken on this PR is probably that CI does not pass. At least that's the main reason for me personally. |
Indeed, I wanted to help investigate the CI failures but forgot and haven't had time. I would probably also expect it to detect the filesystem case sensitivity and act accordingly, not just always assume insensitive is okay, otherwise this change could break it for others, no? |
Sorry, this comment was meant to ask for help on that. My previous PR with CI passing (#466) also hasn't been merged, so I wasn't sure if anyone was looking here.
There are two other locations in the existing code where case insensitivity is being used: syntect/src/parsing/syntax_set.rs Lines 186 to 200 in c61ce60
In other words, this is the only code location where case sensitivity is (incorrectly, imho) required. |
As for file extensions when looking up a syntax, it has been proven that Sublime Text does this in a case insensitive manner, and looking up a syntax for an arbitrary token is just a convenience that syntect offers, and it makes sense to be insensitive. So if we can prove that Sublime also handles varying case |
I was able to reproduce this failing test locally. It seems it is because there is a folder called |
And for what it's worth, I tested Sublime Text 4148 on Linux (i.e. case-sensitive file system) and |
I am not sure if this is a task for me or the repo's administrators, but, Sublime 4143 on macOS (case-insensitive) will happily load a file whether it's |
Cool, so let's fix it to only look for files and you will get my approval :) |
Done! Edit: ah it seems VS Code formatted the file. Happy to undo that if desired. |
Thank you for the contribution. Indeed it is mostly formatting changes. For the record, I am strongly in favor of syntect running cargo fmt as part of CI. That way the problem of PRs containging pure formatting changes goes away completely. It is a very common practice in the Rust ecosystem (and other ecosystems) in my experience, and it works great. It is a time saver both for maintainers and contributors. |
This was an annoying bug to unravel. 😓 On case insensitive systems, like macOS,
tmtheme
ought to be just as valid astmTheme
.