-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_analyze): lint/correctness/noDupeKeys #3562
Conversation
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
There are some things to figure out still, but overall it turned out pretty clean I'd say.
Not sure yet if I'll continue on the weekend or later.
I hit some infra issues that would probably be quickest solved with some help from someone who's already had to deal with similar things, see my comments.
for member in node | ||
.members() | ||
.into_iter() | ||
// Note that we iterate from last to first property, so that we highlight properties being overwritten as problems and not those that take effect. |
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 might be a bit of a hot take, and ESLint does it the other way.
But I think it's the right way, esp. considering the fix suggestion.
* upstream/main: fix(ci): fix the release workflows for the stable release (rome#3583) Fix logo container margin Fix mobile docs regressions perf: End-to-end Linter and Formatter benchmarks (rome#3570) doc: VS Code extension (rome#3579) refactor(rome_cli): refactor the threading of parallel traversal to increase occupancy (rome#3577) [docs] Add navigation dropdown for docs (rome#3578) doc(rome_cli): Document `--files-max-size` option perf(rome_js_semantic): Use FX Hash function (rome#3565) fix(rome_js_analyzer): `noInvalidConstructorSuper` false positive for class expressions (rome#3561) Clean up mobile navigation doc(website): Run `cargo lintdoc` (rome#3567) doc: Fix install command Fix mobile code blocks Fix dark mode logo Update links Implement new website (rome#3556)
…operty definition even in the case of a value being overwritten by a getter and a setter, only highlight the latter of those two
/// Asserts the removal worked. | ||
pub fn assert_remove_ok(before: &str, expected: &str) { | ||
pub fn assert_remove_identifier_a_ok<Anc: AstNode<Language = JsLanguage> + Debug>( |
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 had to generalize this test util a bit so that it can look for any identifier token and then you tell it how much around the token you'd like to remove by giving it an ancestor type to look for
@@ -113,8 +146,7 @@ pub fn ok_find_attributes_by_name() { | |||
let list = r | |||
.syntax() | |||
.descendants() | |||
.filter_map(rome_js_syntax::JsxAttributeList::cast) | |||
.next() | |||
.find_map(rome_js_syntax::JsxAttributeList::cast) |
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.
autofix 😅
|
||
// valid for now | ||
|
||
// ESLint already catches properties keyed with different-formatted number literals, we haven't implemented it yet. |
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 PR has grown pretty big already so I'll leave this for a future extension to the rule 😅
@@ -75,14 +75,15 @@ define_dategories! { | |||
|
|||
|
|||
// nursery | |||
"lint/nursery/useFlatMap": "https://docs.rome.tools/lint/rules/useFlatMap", | |||
"lint/nursery/noBannedTypes":"https://docs.rome.tools/lint/rules/noBannedTypes", |
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.
went ahead and pressed sort on these as there was no apparent order to them
This PR is ready for review now! :) |
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.
Well done. I have a few small nits, and we must wait to merge until the 10.0.1 patch release is out of the door.
* upstream/main: fix(rome_js_analyze): Verify method name of React API calls (rome#3619) benchmark: Add pprettier and dprint to benchmark (rome#3597) feat(vscode): display the version of the language server in the status bar (rome#3616) fix(rome_cli): correctly account for diff diagnostics in the printed diagnostics count (rome#3595) fix(rome_cli): Respect formatter/linter `enabled` from configuration (rome#3591) Remove dead styles [website] Add scroll-padding-top
Co-authored-by: Micha Reiser <[email protected]>
* origin/no-dupe-keys: Update crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs
* upstream/main: fix(npm/js-api): Import type from @rometools/backend-jsonrpc (rome#3647) doc(npm/js-api): Add experimental warning to README fix(npm/js-api): Lazy load backend implementations (rome#3652) feat(rome_lsp): stop the daemon after the last client disconnects (rome#3643) fix(npm/js_api): Ensure JS API build contains `dist` folder (rome#3648) fix(rome_cli): ensures the service only connects to compatible versions (rome#3642) feat(playground): add settings button and group IR (rome#3559) release: v10.0.1 (rome#3649) fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` (rome#3644) fix(rome_js_semantic): support for object and array bindings when exporting (rome#3620) doc(editors): Clarify Rome discovery (rome#3639) fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue` (rome#3626) chore(npm): add license (rome#3629) Add rustdocs index Add environment Change rust docs to use Netlify fix(rome_js_analyze): useValidAnchor considering all possible expressions (rome#3599) fix(rome_js_formatter): Trailing comma inside import rome#3600 (rome#3624)
All comments addressed with changes or explanations why they were left unchanged. |
@MichaReiser I refactored quite a lot and should have addressed your concerns about too wide typings and about blurry naming conventions. Member definitions now clearly use the existing terminology that you mentioned. I also used more impls and traits to perhaps make the core algorithm of the rule itself easier to read. |
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.
We're good to go after you resolved the merge conflicts.
* upstream/main: (45 commits) website(docs): set `color-scheme` on the root element (rome#3721) feat(rome_analyze): add a warning for unused suppression comments (rome#3718) feat(rome_js_analyze): Implement prefer-numeric-literals lint (rome#3558) feat(rome_js_formatter): jestEach template literals rome#3308 (rome#3582) doc(website): Add context about Romes philosophy (rome#3714) fix(rome_js_formatter): Single-line comment below a JSX prop triggers… (rome#3641) test(rome_js_formatter): update prettier tests (rome#3684) fix(rome_js_parser): improve await handling in non-async context (rome#3573) fix(rome_js_parser): improve yield parsing in non generator function (rome#3622) More playground polish Fix backgrounds Fix height Align docs.rome.tools with rome.tools Reenable compression Add missing width website(docs): More playground IDE features (rome#3711) fix(rome_js_formatter): new expression attribute (rome#3686) docs(website): added checkbox to toggle linter in playground (rome#3699) website(docs): More website tweaks (rome#3707) website(docs): Add default layout property (rome#3705) ...
@MichaReiser merged in main! |
Summary
Resolves #3364
Note that I've stopped short of feature parity with ESLint because the PR already got pretty big. The rule can be extended later. I can open a follow up to #3364 when this is merged.
The cases that ESLint covers but this doesn't can be seen in the test file - number-keyed properties and computed-string-literal-keyed properties.
Test Plan
Used standard lint test infrastructure, had to extend slightly for the suggested fix action