8000 feat(rome_json_analyze): noDuplicateKeys by ematipico · Pull Request #4592 · rome/tools · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_json_analyze): noDuplicateKeys - #4592

Merged
ematipico merged 9 commits into
mainfrom
feat/json-analyze
Jun 21, 2023
Merged

feat(rome_json_analyze): noDuplicateKeys#4592
ematipico merged 9 commits into
mainfrom
feat/json-analyze

Conversation

@ematipico
@ematipico ematipico commented Jun 18, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Summary

Part of #4444

I created a new crate and rule under the nursery group. Updated all the configuration scripts too.

Test Plan

Added new test cases.

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
netlify Bot commented Jun 18, 2023 •
edited
Loading
Copy link
Copy Markdown

✅ Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 6cf78d5
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6492e2ed78c6c90008eedb0c
😎 Deploy Preview https://deploy-preview-4592--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions Bot added A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling labels Jun 18, 2023
@ematipico
ematipico marked this pull request as ready for review June 18, 2023 16:49
@ematipico
ematipico force-pushed the feat/json-analyze branch from 722c212 to 81116af Compare June 18, 2023 16:50
@github-actions
github-actions Bot commented Jun 18, 2023
Copy link
Copy Markdown

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47810 47810 0
Failed 1053 1053 0
Panics 0 0 0
Coverage 97.84% 97.84% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1763 1763 0
Failed 4449 4449 0
Panics 0 0 0
Coverage 28.38% 28.38% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13121 13121 0
Failed 4103 4103 0
Panics 0 0 0
Coverage 76.18% 76.18% 0.00%

@ematipico
ematipico force-pushed the feat/json-analyze branch from 70ccab4 to 2df6361 Compare June 19, 2023 07:33
@ematipico
ematipico requested a review from Conaclos June 19, 2023 08:26
@ematipico
ematipico force-pushed the feat/json-analyze branch 2 times, most recently from 2b09f59 to b58acb2 Compare June 19, 2023 08:41
Conaclos
Conaclos approved these changes Jun 19, 2023
View reviewed changes
@Conaclos Conaclos left a comment •
edited
Loading
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.

While we keep adding rules, I am wondering how a user knows if a rule is for a language A or a language B.

For instance, the current rule noDuplicateKeys could easily be confused with noDuplicateObjectKeys. Should we rename it to noDuplicateJsonKeys?

EDIT: However, this could result in adding Json in any JSON lint rules?

Comment thread crates/rome_json_analyze/src/analyzers/nursery/no_duplicate_keys.rs Resolved
Comment thread crates/rome_json_analyze/src/analyzers/nursery/no_duplicate_keys.rs Resolved
Comment thread crates/rome_json_analyze/src/analyzers/nursery/no_duplicate_keys.rs Resolved
Comment thread crates/rome_json_analyze/src/analyzers/nursery/no_duplicate_keys.rs Resolved
denbezrukov
denbezrukov reviewed Jun 19, 2023
View reviewed changes
Comment thread crates/rome_json_analyze/tests/spec_tests.rs Resolved
@ematipico
ematipico commented Jun 21, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

While we keep adding rules, I am wondering how a user knows if a rule is for a language A or a language B.

That's the good thing about that rome, a rule is language agnostic. In the previous version of rome (the TS one), some rules were able to run in JSX and HTML (mainly the a11y ones) :)

I believe we are not there yet though, in terms of architecture.

@ematipico
ematipico force-pushed the feat/json-analyze branch from b58acb2 to 416cd37 Compare June 21, 2023 09:22
@Conaclos
Conaclos commented Jun 21, 2023
Copy link
Copy Markdown
Contributor

Since we have noDuplicateJsxProps, I think it could be good to rename the rule to noDuplicateJsonKeys.

@ematipico
ematipico requested a review from Conaclos June 21, 2023 11:45
@ematipico
ematipico commented Jun 21, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

Going to merge this. I'll follow up with a PR to integrate it with the workspace, and I am going to add some documentation too. Changelog as well, obviously

@ematipico
ematipico merged commit 50eb45f into main Jun 21, 2023
@ematipico
ematipico deleted the feat/json-analyze branch June 21, 2023 13:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@denbezrukov denbezrukov denbezrukov left review comments
@Conaclos Conaclos Conaclos 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-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ematipico @Conaclos @denbezrukov
0