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

fix(configuration): enabled rules calculation #2072

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

Sec-ant
Copy link
Member

@Sec-ant Sec-ant commented Mar 13, 2024

Summary

It turns out the existing function as_enabled_rules was calculating the enabled rules in a wrong way. The function is generated by codegen, so I fixed the logic in xtask/codegen/src/generate_configuration.rs and regenerated that file. I summarize the correct logic as follows:

For non-nursery groups

  • Group-level all and group-level recommend should take precedence over their top-level alternatives.
  • When group-level all or group-level recommend is not present, it should fallback to check top-level all or top-level recommend.
  • If the whole group-level config is not present, it should check top-level all or top-level recommend.

For nursery group

  • Group-level all and group-level recommend should take precedence over top-level alternatives. (same as non-nursery groups)
  • When group-level all or group-level recommend is not present, it should fallback to check top-level all or top-level recommend only in unstable releases.
  • If the whole group-level config is not present, it should check top-level all or top-level recommend only in unstable releases.

Fixes #2028.

Test Plan

Since the nursery group is unstable because the rules in it may be promoted in later releases, I didn't add test cases for the nursery group. But I tested it locally and it works as expected.

I added a test case called top_level_all_down_level_empty. It's to check whether the non-recommended rules belonging to a group will be enabled if we have a top-level all as true and an empty config of that group. The expected result is they should be kept enabled. In current release (v1.6.1) they are wrongly disabled. I guess the reason why this issue isn't discovered earlier is because most rules under a group are recommended rules.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Tooling Area: internal tools labels Mar 13, 2024
Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 3581368
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65f1953e2ca1140008d4c7f6
😎 Deploy Preview https://deploy-preview-2072--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🔴 down 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@Conaclos
Copy link
Member

LGTM. It could be great to add a changelog entry.
You have to add the Unreleased section. See our contributing guide for more details :)

@Sec-ant Sec-ant force-pushed the fix-rule-calculation branch from da33bf6 to 3581368 Compare March 13, 2024 11:59
@github-actions github-actions bot added A-Website Area: website A-Changelog Area: changelog labels Mar 13, 2024
@Conaclos Conaclos merged commit 5bc24f1 into biomejs:main Mar 13, 2024
17 of 18 checks passed
@Sec-ant Sec-ant deleted the fix-rule-calculation branch March 13, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Project Area: project A-Tooling Area: internal tools A-Website Area: website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💅 Regression: Disabling one nursery rule disables all nursery rules
2 participants