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): don't conceal a previous override with the base settings #3183

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Jun 11, 2024

Summary

Fix #3176

When we convert the Biome Configuration to Workspace settings, we use the base settings to set the missing settings in every override. This results in overrides that conceal previous overrides.

For example, the following config:

  {
    "linter": {
      "rules": {
        "suspicious": { "noDebugger": "off" }
      }
    },
    "overrides": [
      {
        "include": ["index.js"],
        "linter": {
          "rules": {
            "suspicious": { "noDebugger": "warn" }
          }
        }
      }, {
        "include": ["index.js"]
      }
    ]
  }

was resolved to:

  {
    "linter": {
      "rules": {
        "suspicious": { "noDebugger": "off" }
      }
    },
    "overrides": [
      {
        "include": ["index.js"],
        "linter": {
          "rules": {
            "suspicious": { "noDebugger": "warn" }
          }
        }
      }, {
        "include": ["index.js"],
        "linter": {
          "rules": {
            "suspicious": { "noDebugger": "off" }
          }
        }
      }
    ]
  }

The current PR fixes the issue by removing the propagation of the base configuration to every override.

I also fixed several erroneous implementations.
For example, we took the first override.{linter.formatter.organizeImports}.enabled into account, while we have to take the last one.

I think there are still some issues. Some fields such as jsx_runtime are not behind an Option in overrides.
This means that every overrides has a default value for this field.
This is a change for a future PR.

Test Plan

I added new tests to avoid any regressions.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Changelog Area: changelog labels Jun 11, 2024
@Conaclos Conaclos force-pushed the conaclos/overrides-linter-fix branch 5 times, most recently from bf317eb to a9eae27 Compare June 12, 2024 10:15
@Conaclos Conaclos marked this pull request as ready for review June 12, 2024 10:16
@Conaclos Conaclos requested review from a team June 12, 2024 10:16
@Conaclos Conaclos force-pushed the conaclos/overrides-linter-fix branch from 80d1b83 to a9eae27 Compare June 12, 2024 11:23
@Sec-ant Sec-ant force-pushed the conaclos/overrides-linter-fix branch from a9eae27 to 5e9b6eb Compare June 12, 2024 12:41
Copy link
Member

@Sec-ant Sec-ant left a comment

Choose a reason for hiding this comment

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

Sorry I tried to rebase and update the snapshots, but I forgot to accept them before commiting them. 🥺

Fixed

@Sec-ant Sec-ant force-pushed the conaclos/overrides-linter-fix branch from 5e9b6eb to d94a5c2 Compare June 12, 2024 13:16
Comment on lines +772 to +774
// Reverse the traversal as only the last override takes effect
.rev()
.find_map(|pattern| {
Copy link
Member

Choose a reason for hiding this comment

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

Damn, I will never understand overrides, my brain just fails to get them 😓

@Conaclos Conaclos merged commit b5d3692 into main Jun 12, 2024
13 checks passed
@Conaclos Conaclos deleted the conaclos/overrides-linter-fix branch June 12, 2024 16:27
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Override behavior with multiple matches
3 participants