Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(graphql_analyze): noDuplicatedFields #3308

Merged

Conversation

vohoanglong0107
Copy link
Contributor

Summary

Implement no-duplicate-fields lint rule from graphql-eslint

Test Plan

All tests should pass.

@github-actions github-actions bot added A-Project Area: project A-Diagnostic Area: diagnostocis labels Jun 29, 2024
Copy link

codspeed-hq bot commented Jun 29, 2024

CodSpeed Performance Report

Merging #3308 will degrade performances by 6.23%

Comparing vohoanglong0107:feat-no-duplicated-fields (2bf29fb) with main (8834e26)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 105 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main vohoanglong0107:feat-no-duplicated-fields Change
dojo_11880045762646467684.js[cached] 8.6 ms 8 ms +6.47%
router_14177007973872119684.ts[cached] 2.3 ms 2.1 ms +6.71%
db_2930068967297060348.json[cached] 12.5 ms 13.3 ms -6.23%

@vohoanglong0107 vohoanglong0107 force-pushed the feat-no-duplicated-fields branch from 911f2a3 to f125808 Compare June 29, 2024 07:03
Comment on lines 42 to 47
version: "next",
name: "noDuplicatedGraphqlFields",
language: "graphql",
recommended: true,
Copy link
Member

Choose a reason for hiding this comment

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

Since this rule comes from an existing one, you should add the source: https://github.com/biomejs/biome/blob/main/crates/biome_analyze/CONTRIBUTING.md#biome-lint-rules-inspired-by-other-lint-rules

Not sure if we should do the same for the other rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I forgot about that. The other rule also has an equivalent version in graphql-eslint, I guess it makes sense to also add a source for it

@github-actions github-actions bot added the A-Linter Area: linter label Jun 29, 2024
@vohoanglong0107 vohoanglong0107 marked this pull request as ready for review June 29, 2024 13:39
Copy link
Member

@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.

You'll have to rebase, and change declare_rule to declare_lint_rule

@ematipico ematipico force-pushed the feat-no-duplicated-fields branch from f1f6d11 to 63da6c3 Compare July 4, 2024 07:27
@vohoanglong0107
Copy link
Contributor Author

Ah, sorry I didn't see your comment. Thanks for rebasing this for me

@ematipico
Copy link
Member

Just one nit @vohoanglong0107, we should avoid having the language name inside the rule's name. We already have the metadata that tells us the language.

@vohoanglong0107
Copy link
Contributor Author

I'm a little confused about this, but what if there is another rule for JSON called noDuplicatedFields, how could we config both of them?

@ematipico
Copy link
Member

That's the beauty of this analyzer. A rule should work across languages, and the infrastructure will take care of it. Imagine the a11y rules, they need to work both in JSX and HTML.

The infrastructure and the website still needs some final touches, but we'll get there.

@vohoanglong0107
Copy link
Contributor Author

Ahah, I understand. I will keep this in mind when developing further lint rules

@vohoanglong0107 vohoanglong0107 force-pushed the feat-no-duplicated-fields branch from 63da6c3 to 2bf29fb Compare July 4, 2024 08:50
@vohoanglong0107 vohoanglong0107 changed the title feat(graphql_analyze): noDuplicatedGraphqlFields feat(graphql_analyze): noDuplicatedFields Jul 4, 2024
@ematipico
Copy link
Member

Feel free to merge it :)

@vohoanglong0107 vohoanglong0107 merged commit e36899a into biomejs:main Jul 8, 2024
12 of 13 checks passed
@vohoanglong0107 vohoanglong0107 deleted the feat-no-duplicated-fields branch July 8, 2024 08:13
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants