-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(lsp): allow formatting vendor files #20844
Conversation
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.
Does deno fmt ./vendor/esm.sh/v124/@twind/core@1.1.3/deno/core.mjs
from the issue work? I think maybe this doesn't close the issue entirely?
.url_map | ||
.normalize_url(¶ms.text_document.uri, LspUrlKind::File); | ||
// skip formatting any files ignored by the config file | ||
if !self.fmt_options.files.matches_specifier(&specifier) { |
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.
If someone ignores something in the vendor folder, will this also skip those files? I think probably not. Maybe it should?
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.
Currently vendor
isn't applicable to config exclusion checks because it's excluded by default. We should remove https://github.com/denoland/deno_config/blob/0.4.0/src/lib.rs#L688-L690 for a start. It shouldn't be handled there.
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.
LGTM with some caveats.
We should change our fmt config exclusion check to only apply to automatic discovery such as |
I'm gonna land this for now and open more targeted issues for the remainders |
Fixes #20308.