8000 feat(rome_js_analyze): new lint rule: `useImportRestrictions` by arendjr · Pull Request #4700 · rome/tools · GitHub
[go: up one dir, main page]
Skip to content 1 10000
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

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

Merged
ematipico merged 9 commits into
rome:mainfrom
arendjr:main
Jul 21, 2023
Merged

feat(rome_js_analyze): new lint rule: useImportRestrictions#4700
ematipico merged 9 commits into
rome:mainfrom
arendjr:main

Conversation

@arendjr
@arendjr arendjr commented Jul 14, 2023
Copy link
Copy Markdown
Contributor

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
netlify Bot commented Jul 14, 2023 •
edited
Loading
Copy link
Copy Markdown

❌ 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
arendjr
arendjr commented Jul 15, 2023
View reviewed changes
Comment thread crates/rome_console/src/fmt.rs Resolved
ematipico
ematipico suggested changes Jul 17, 2023
View reviewed changes
@ematipico ematipico left a comment
Copy link
Copy Markdown
Contributor

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.

Comment thread Cargo.toml Resolved
Comment thread crates/rome_console/src/fmt.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
@arendjr
arendjr commented Jul 18, 2023
Copy link
Copy Markdown
Contributor Author

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

@ematipico
ematipico commented Jul 21, 2023
Copy link
Copy Markdown
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
@ematipico ematipico left a comment
Copy link
Copy Markdown
Contributor

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!

Comment thread crates/rome_js_analyze/tests/specs/nursery/useImportRestrictions/validPackagePrivateImports.js Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Resolved
Comment thread 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"];
@ematipico ematipico Jul 21, 2023
Copy link
Copy Markdown
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 :)

@arendjr arendjr Jul 21, 2023
Copy link
Copy Markdown
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
arendjr commented Jul 21, 2023
Copy link
Copy Markdown
Contributor Author

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

1 more reviewer
@ematipico ematipico ematipico approved these changes
Reviewers whose approvals may not affect merge requirements

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
0