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(lsp): allow formatting vendor files #20844

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

nayeemrmn
Copy link
Collaborator

Fixes #20308.

Copy link
Member

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/[email protected]/deno/core.mjs from the issue work? I think maybe this doesn't close the issue entirely?

.url_map
.normalize_url(&params.text_document.uri, LspUrlKind::File);
// skip formatting any files ignored by the config file
if !self.fmt_options.files.matches_specifier(&specifier) {
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

@dsherret dsherret left a 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.

@nayeemrmn
Copy link
Collaborator Author

We should change our fmt config exclusion check to only apply to automatic discovery such as deno fmt <encompassing-dir> and formatOnSave (though we can't detect that one unfortunately).

@nayeemrmn
Copy link
Collaborator Author

I'm gonna land this for now and open more targeted issues for the remainders

@nayeemrmn nayeemrmn merged commit 84c9300 into denoland:main Oct 9, 2023
13 checks passed
@nayeemrmn nayeemrmn deleted the lsp-format-vendor-file branch October 9, 2023 22:46
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.

Bug: Unable to format vendored files
2 participants