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(config): group-level lint rules calculation #2204

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

Sec-ant
Copy link
Member

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

Summary

Correctly calculate enabled rules in lint rule groups.

This is the Karnaugh map of lint group preset logic:

Self (A, R) \ Parent (A, R) (N, F) (N, T) (F, F) (F, T) (T, F) (T, T)
(N, N) - R - R A A
(N, F) - - - - A A
(N, T) R R R R A A
(F, N) - R - R - -
(F, F) - - - - - -
(F, T) R R R R R R
(T, N) A A A A A A
(T, F) A A A A A A
(T, T) A A A A A A

A: all, R: recommended, N: None, T: true, F: false

And the code to implement the above logic is:

  if self.is_all() || self.is_all_unset() && parent_is_all {
      enabled_rules.extend(Self::all_rules_as_filters());
  } else if self.is_recommended() || self.is_recommended_unset() && parent_is_recommended && !parent_is_all {
      enabled_rules.extend(Self::recommended_rules_as_filters());
  }

When calculating the preset rules, we shouldn't populate the disabled rules set, because that will make specific rules cannot be enabled later.

Closes #2191.

Test Plan

Added a test case borrowed from #2191.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Tooling Area: internal tools A-Website Area: website A-Changelog Area: changelog labels Mar 25, 2024
Copy link

netlify bot commented Mar 25, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit a9ee6c0
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/660232d169834200083f2fe6
😎 Deploy Preview https://deploy-preview-2204--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.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -488,16 +487,16 @@ fn generate_struct(group: &str, rules: &BTreeMap<&'static str, RuleMetadata>) ->
matches!(self.recommended, Some(true))
}

pub(crate) const fn is_not_recommended(&self) -> bool {
matches!(self.recommended, Some(false))
pub(crate) const fn is_recommended_unset(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that! Naming is hard, and it's these little touches that can make a difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you but here is a different case. is_not_recommended tests Some(false) while is_recommended_unset tests None. To implement the logic I created the latter one, and the former one is no longer needed in the code, hence the change.

To make the namings more consistent I probably should've renamed is_recommended to is_recommended_true. But as Ema suggests these preset options will be dropped in v2.0 so I didn't touch them.

@Sec-ant Sec-ant merged commit d86bb50 into biomejs:main Mar 26, 2024
16 checks passed
@Sec-ant Sec-ant deleted the fix/rule-precedence branch March 26, 2024 02:48
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.

💅 recommended: false disables all nested rules
3 participants