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

chore(editorconfig): reenable editorconfig loading #2971

Closed
wants to merge 1 commit into from

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 24, 2024

Summary

This was disabled in #2943.

related to: #1724

Test Plan

CI should pass, and pnpm check:apply should work without throwing errors

@github-actions github-actions bot added the A-CLI Area: CLI label May 24, 2024
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.

We can't enable it until we have a configuration.

This very repository runs biome check, and it fails because the editorconfig throws a parsing error. That's the reason why I disabled in the first place

I see two options:

  • add the config
  • print only warnings

@dyc3
Copy link
Contributor Author

dyc3 commented May 24, 2024

Ah, ok, I misunderstood. I'll mark this as draft until the config gets added.

It's likely emitting diagnostics for this (but they should only be warnings):

[{**.yml,**.md,**.rs,**.mdx,justfile,**.json}]
indent_style = space

Which is intentional, because biome can't parse these kinds of glob patterns. The easy fix for this would be to just separate this override into multiple overrides. Would we want to support this glob pattern syntax?

@dyc3 dyc3 marked this pull request as draft May 24, 2024 16:24
@ematipico
Copy link
Member

Would we want to support this glob pattern syntax?

Definitely, yes!

@dyc3
Copy link
Contributor Author

dyc3 commented May 26, 2024

I've updated this PR to change the editorconfig so that it can be parsed by biome. Which should allow the check:apply script to run without emitting errors, but I can't test it myself without the shared config.

@dyc3
Copy link
Contributor Author

dyc3 commented May 30, 2024

Closing this because this conflicts with another PR I'm working on.

@dyc3 dyc3 closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants