-
Notifications
You must be signed in to change notification settings - Fork 72
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
vscode-stylua: search configuration file for Lua files not in workspace #735
base: main
Are you sure you want to change the base?
Conversation
Can you explain the use-case of this and what it's solving? This seems like it's probably better solved by changing the StyLua binary to search for configuration recursively upward rather than adding an additional one-level-up check in the extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, wait a second, I might be wrong here.
This does fix one case, but then breaks another case.
Right now the extension works by using the root workspace folder as the cwd.
That works for the following (typical) case:
foo/
bar.lua
baz.lua
stylua.toml
Your PR I assume is meant to fix the following case:
foo/
bar.lua
stylua.toml
baz.lua
Where we are formatting bar.lua using the nested stylua.toml
This, however, will break the original typical case, since Stylua by default does not search parent directories
Hi, thanks for reviewing the PR! It won't break the original case if The typical use case here is that suppose you're working in the workspace The current code that use To resolve your example, I think the better solution is to set |
This has unintended side-effects, which is why the setting is not enabled by default in the first place. I can see the issue the PR is trying to solve. What if instead we check if the file we are trying to format falls outside of the current workspace, then apply this change? If yes, then use its parent directory as the cwd. If no, then use the current workspace as cwd, as it does right now. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 16 16
Lines 6119 6119
=======================================
Hits 5972 5972
Misses 147 147 ☔ View full report in Codecov by Sentry. |
@JohnnyMorganz: I just updated the PR according to your suggestion. Can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it should work, thank you! I haven't tested it myself yet - did it work as needed for you?
Hi,
The current VSCode StyLua extension is unable to search for configuration files in parent directories of a Lua file not in the current workspace.
I fixed that issue by setting the
cwd
parameter to the parent directory of the currently opened file.Can you consider merging this PR?
Thanks!