Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): new lint rule: useImportRestrictions #4700

Merged
merged 9 commits into from
Jul 21, 2023
Merged

feat(rome_js_analyze): new lint rule: useImportRestrictions #4700

merged 9 commits into from
Jul 21, 2023

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Jul 14, 2023

Summary

This PR implements the initial version of the new useImportRestrictions as discussed in #4678

I think I have taken all the feedback from the previous PR into account now.

Test Plan

Tests are included in the PR.

@netlify
Copy link

netlify bot commented Jul 14, 2023 •
edited
Loading

❌ Deploy Preview for docs-rometools failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit 4b62c7b
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64ba818627dac6000821cd51

@github-actions github-actions bot added A-Linter Area: linter A-Project Area: project configuration and loading A-Diagnostic Area: errors and diagnostics labels Jul 14, 2023
ematipico
ematipico suggested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I left some suggestions around:

  • documentation
  • implementation

Documentation: it should be more beginner-friendly
Implementation: using PathBuf add complexity that we don't need. I believe working with strings is a more suitable approach where we don't need to deal with OS specific stuff.

Cargo.toml Show resolved Hide resolved
crates/rome_console/src/fmt.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
@arendjr
Copy link
Contributor Author

arendjr commented Jul 18, 2023

I think all the PR issues have been addressed, just need to know what to do with the Cargo.toml :)

@ematipico
Copy link
Contributor

I think all the PR issues have been addressed, just need to know what to do with the Cargo.toml :)

Nothing, that's the tool doing its job. Thank you!

ematipico
ematipico approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, and then we can merge!

crates/rome_js_analyze/tests/specs/nursery/useImportRestrictions/validPackagePrivateImports.js Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs

const INDEX_BASENAMES: &[&str] = &["index", "mod"];

const SOURCE_EXTENSIONS: &[&str] = &["js", "ts", "cjs", "cts", "mjs", "mts", "jsx", "tsx"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider .d.ts too? Genuine question, I don't know it :)

Copy link
Contributor 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 it makes sense to be honest, since you can't import "some_file.d.ts".

@github-actions github-actions bot added A-Website Area: website and documentation L-JavaScript Langauge: JavaScript L-CSS Language: CSS A-Parser Area: parser labels Jul 21, 2023
@arendjr
Copy link
Contributor Author

arendjr commented Jul 21, 2023

All done 👍

@ematipico ematipico merged commit d496da0 into rome:main Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading A-Website Area: website and documentation L-CSS Language: CSS L-JavaScript Langauge: JavaScript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arendjr @ematipico