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

🐛 Formatter ignore doesn't work #1465

Closed
1 task done
anonrig opened this issue Jan 8, 2024 · 7 comments · Fixed by #1489
Closed
1 task done

🐛 Formatter ignore doesn't work #1465

anonrig opened this issue Jan 8, 2024 · 7 comments · Fixed by #1489
Assignees
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@anonrig
Copy link
Contributor

anonrig commented Jan 8, 2024

Environment information

CLI:
  Version:                      1.5.0
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "alacritty"
  JS_RUNTIME_VERSION:           "v18.17.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/9.6.7"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 false

Workspace:
  Open Documents:               0

What happened?

  1. Clone https://github.com/getsentry/sentry-javascript
  2. Update Biome to v1.5.0
  3. Run biome check --apply .
The number of diagnostics exceeds the number allowed by Biome.
Diagnostics not shown: 640.
Checked 2412 file(s) in 180ms
Found 190 error(s)

Expected result

It should not format dev-packages/browser-integration-tests/fixtures/loader.js because of this line: https://github.com/getsentry/sentry-javascript/blob/develop/biome.json#L46

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico
Copy link
Member

Can you try with **/dev-packages/browser-integration-tests/fixtures/loader.js?

@anonrig
Copy link
Contributor Author

anonrig commented Jan 8, 2024

Can you try with **/dev-packages/browser-integration-tests/fixtures/loader.js?

@ematipico This works, but this results in a breaking-change.

I've also opened a discussion around this: #1466

@Conaclos
Copy link
Member

Conaclos commented Jan 8, 2024

Can you try with **/dev-packages/browser-integration-tests/fixtures/loader.js?

@ematipico This works, but this results in a breaking-change.

I've also opened a discussion around this: #1466

Indeed, this is a mistake in our side.
We should support the old way of ignoring files and emits a deprecation warning before removing this in a major release (Biome v2).

@ematipico ematipico added A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug labels Jan 9, 2024
@ematipico ematipico self-assigned this Jan 9, 2024
@Conaclos
Copy link
Member

@anonrig this should be fixed in the last patch release (v1.5.1).

@anonrig
Copy link
Contributor Author

anonrig commented Jan 10, 2024

@Conaclos I'm still having some weird error messages, and suggestions on how to diagnose the root cause?

$ biome check .
The number of diagnostics exceeds the number allowed by Biome.
Diagnostics not shown: 200.
Checked 2413 file(s) in 3s
Found 3 error(s)
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.


error Command failed with exit code 1.

@Conaclos
Copy link
Member

Thanks for your test.

It seems that the verbose diagnostics are counted as regular diagnostics. Because they come first, they hide useful diagnostics.

If you raise the number of allowed diagnostics, Biome will report code that should be reformatted:

❯ npx @biomejs/biome format --max-diagnostics=500 .

./packages/nextjs/src/common/wrapPageComponentWithSentry.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ℹ Formatter would have printed the following content:
  
     7  7 │   
     8  8 │   interface ClassComponent {
     9    │ - ··new·(...args:·unknown[]):·{
        9 │ + ··new·(
       10 │ + ····...args:·unknown[]
       11 │ + ··):·{
    10 12 │       props?: unknown;
    11 13 │       render(...args: unknown[]): unknown;
  

I also found a bug I reported in #1511.
This requires another patch version.

@Conaclos
Copy link
Member

@anonrig The 1.5.2 release removes the remaining regressions introduced in 1.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants