-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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(unstable): move sloppy-import warnings to lint rule #24710
Conversation
} | ||
} | ||
|
||
fn apply_lint_fixes_and_relint( |
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.
Moved from cli/tools/mod.rs
source_code: String, | ||
file_path: &Path, | ||
) -> Result<(ParsedSource, Vec<LintDiagnostic>), deno_core::anyhow::Error> { | ||
// initial lint |
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.
Contents moved from cli/tools/lint/mod.rs
file_path: &Path, | ||
source_code: String, | ||
) -> Result<(ParsedSource, Vec<LintDiagnostic>), AnyError> { | ||
let specifier = specifier_from_file_path(file_path)?; |
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.
Contents moved from cli/tools/lint/mod.rs
!self.package_rules.is_empty() | ||
} | ||
|
||
pub fn lint_package( |
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.
This is also incrementally working towards having these lint rules show up in the lsp.
probe_media_type_types: Vec<MediaType>, | ||
reason: SloppyImportsResolutionReason, | ||
) -> Vec<(PathBuf, SloppyImportsResolutionReason)> { | ||
probe_media_type_types | ||
.into_iter() | ||
.filter(|media_type| *media_type != original_media_type) |
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.
Fixes a bug with sloppy imports.
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.
A few comments, but overall looks good to me 👍
let linter = Arc::new(CliLinter::new(CliLinterOptions { | ||
configured_rules: lint_rules, | ||
fix: lint_options.fix, | ||
deno_lint_config: lint_config, | ||
})); |
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.
It's most likely a problem for another time, but we're gonna have a lot of "fun" with this abstraction if we start supporting plugins. Again, a problem for another time.
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.
I don't think this makes that any harder. Maybe easier.
Enforces specifying explicit references to paths in module specifiers. | ||
|
||
Non-explicit specifiers are ambiguous and require probing for the correct file | ||
path on every run, which has a performance overhead. | ||
|
||
Note: This lint rule is only active when using `--unstable-sloppy-imports`. | ||
|
||
### Invalid: | ||
|
||
```typescript | ||
import { add } from "./math/add"; |
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.
Cool that this will be showed in doc help output, but it won't be shown on https://lint.deno.land/. Any ideas how we could do that?
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 I think we don't have markdown help text output for specific rules anymore in the deno
binary...
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.
I think we need to move lint.deno.land out of the deno_lint repo and have it automatically update on publish of the CLI. I opened denoland/deno_lint#1300
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
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.
Nice, really looking forward to this!
Adds a new `no-sloppy-imports` lint rule and cleans up the lint code. Closes #22844 Closes denoland/deno_lint#1293
Adds a new
no-sloppy-imports
lint rule and cleans up the lint code.Closes #22844
Closes denoland/deno_lint#1293