Skip to content
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

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
Comment thread crates/rome_console/src/fmt.rs Outdated

@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
Comment thread crates/rome_console/src/fmt.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
Comment thread crates/rome_js_analyze/src/analyzers/nursery/use_import_restrictions.rs Outdated
@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

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 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/src/analyzers/nursery/use_import_restrictions.rs Outdated

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

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

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 :)

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.

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

Development

Successfully merging this pull request may close these issues.

2 participants