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(formatter): don't let overrides erroneously enable formatter if unspecified #2957

Merged
merged 1 commit into from
May 24, 2024

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 23, 2024

Summary

Previously, overrides could erroneously enable the formatter if it was supposed to be disabled, specifically in configurations like this:

{
  "formatter": {
    "enabled": true,
    "lineWidth": 20
  },
  "javascript": {
    "formatter": {
      "enabled": false
    }
  },
  "overrides": [
    { "include": ["formatted.js"], "formatter": { "enabled": true } },
    {
      // this override doesn't have a formatter section, so it would assume the global value, ignoring the language config
      "include": ["dirty.js"],
      "linter": {
        "rules": {
          "performance": {
            "noBarrelFile": "off"
          }
        }
      }
    }
  ]
}

This PR fixes that behavior by not forcing the override settings to have a Some value no matter what. I briefly looked into whether or not this affects the linter too, and I think it does, but I think that fix is outside the scope of this PR. This area of the code is likely going to be significantly changed by work on #2228. It's somewhat unclear what the semantics of overrides should be (if 2 overrides match the same file, do they get merged? do we take the first matching override and ignore the rest? the last one? etc etc)

Fixes #2924

Test Plan

Added a test with snapshots

@dyc3 dyc3 marked this pull request as draft May 23, 2024 15:01
@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project labels May 23, 2024
@dyc3 dyc3 force-pushed the weird-formatter-disable branch from 1063e46 to 07cbde8 Compare May 23, 2024 15:02
@dyc3 dyc3 marked this pull request as ready for review May 23, 2024 15:11
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.

I think the fix is correct. It feels weird, but unfortunately, it's a side effect of our weird defaults.

@ematipico ematipico merged commit a49bd8d into biomejs:main May 24, 2024
11 checks passed
@ematipico
Copy link
Member

I was too hasty. @dyc3 would you like to send a PR with a changelog line?

@dyc3
Copy link
Contributor Author

dyc3 commented May 24, 2024

Can do: #2969

dyc3 added a commit to dyc3/biome that referenced this pull request May 24, 2024
@dyc3 dyc3 deleted the weird-formatter-disable branch May 24, 2024 12:44
@Conaclos Conaclos added the A-Changelog Area: changelog label May 24, 2024
Conaclos pushed a commit that referenced this pull request May 24, 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-CLI Area: CLI A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📝 disabling formatter doesn't work
3 participants