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(unstable): move sloppy-import warnings to lint rule #24710

Merged
merged 24 commits into from
Jul 25, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jul 24, 2024

Adds a new no-sloppy-imports lint rule and cleans up the lint code.

Closes #22844
Closes denoland/deno_lint#1293

}
}

fn apply_lint_fixes_and_relint(
Copy link
Member Author

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

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)?;
Copy link
Member Author

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(
Copy link
Member Author

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)
Copy link
Member Author

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.

Copy link
Member

@bartlomieju bartlomieju left a 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 👍

cli/tools/lint/rules/mod.rs Show resolved Hide resolved
Comment on lines +288 to +292
let linter = Arc::new(CliLinter::new(CliLinterOptions {
configured_rules: lint_rules,
fix: lint_options.fix,
deno_lint_config: lint_config,
}));
Copy link
Member

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.

Copy link
Member Author

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.

Comment on lines +1 to +11
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";
Copy link
Member

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?

Copy link
Member

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...

Copy link
Member Author

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

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

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

@dsherret dsherret merged commit 763f05e into denoland:main Jul 25, 2024
17 checks passed
@dsherret dsherret deleted the sloppy_imports_lint_rule branch July 25, 2024 13:08
dsherret added a commit that referenced this pull request Jul 26, 2024
Adds a new `no-sloppy-imports` lint rule and cleans up the lint code.

Closes #22844
Closes denoland/deno_lint#1293
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.

feature request: no-sloppy-imports Move sloppy import warnings to lint rule
4 participants