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

vscode-stylua: search configuration file for Lua files not in workspace #735

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

taquangtrung
Copy link

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!

@LastTalon
Copy link
Contributor

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.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a 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!

stylua-vscode/src/extension.ts Outdated Show resolved Hide resolved
Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a 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

@taquangtrung
Copy link
Author

Hi, thanks for reviewing the PR!

It won't break the original case if stylua.searchParentDirectories is set to true.

The typical use case here is that suppose you're working in the workspace ~/workspace-1 but need to edit a Lua file in a folder outside it, say ~/workspace-2/foo/bar.lua and the configuration file is ~/workspace-2/.stylua.toml

The current code that use ~/workkspace-1 as CWD won't be able to format ~/workspace-2/foo/bar.lua

To resolve your example, I think the better solution is to set stylua.searchParentDirectories to true, in addition to the new changes in this PR.

@JohnnyMorganz
Copy link
Owner

To resolve your example, I think the better solution is to set stylua.searchParentDirectories to true, in addition to the new changes in this PR.

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
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (0fb1c15) 97.59% compared to head (d936951) 97.59%.
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@taquangtrung
Copy link
Author

@JohnnyMorganz: I just updated the PR according to your suggestion. Can you take a look?

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a 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?

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.

3 participants