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(biome): run all enabled biome fixers #4763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redbmk
Copy link

@redbmk redbmk commented Apr 24, 2024

  • based on biome config, will format, lint, and/or sort imports
  • adds variable to apply unsafe fixes, disabled by default

fixes: #4754

@redbmk
Copy link
Author

redbmk commented Apr 29, 2024

@w0rp I have a few other fixes queued up for biome, I just need to make sure I have tests for them and everything. Should I make it all part of one PR or keep them more atomic with separate PRs?

@Spixmaster
Copy link
Contributor

@redbmk I highly doubt that waiting this long for a response is worth it. Maybe you can just handle it how you like.

I appreciate your work!

@hsanson
Copy link
Contributor

hsanson commented May 8, 2024

Smaller self-contained PRs are easier to review given limited time maintainers have. Also easier to revert in case something breaks.

let l:apply = '--apply'

let l:unsafe = ale#Var(a:buffer, 'biome_fixer_apply_unsafe')
if (l:unsafe is 1 || l:unsafe is v:true)
Copy link
Contributor

@hsanson hsanson May 8, 2024

Choose a reason for hiding this comment

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

No need to check both 1 and v:true. Simply using if (l:unsafe) should suffice the check. You can make it single line like this:

let l:apply = ale#Var(a:buffer, 'biome_fixer_apply_unsafe') ? '--apply-unsafe' : '--apply'

@hsanson
Copy link
Contributor

hsanson commented May 9, 2024

There is a linter issue that causes the test to fail. After that is fixed I can merge this:

./autoload/ale/fixers/biome.vim:7 Blank line required before `if`.

@redbmk
Copy link
Author

redbmk commented May 9, 2024

There is a linter issue that causes the test to fail. After that is fixed I can merge this:

./autoload/ale/fixers/biome.vim:7 Blank line required before `if`.

@hsanson Cool I just pushed up a fix that included your oneliner suggestion. That's a lot cleaner, thanks! I checked that linters are passing locally now

- based on biome config, will format, lint, and/or sort imports
- adds variable to apply unsafe fixes, disabled by default

fixes: dense-analysis#4754
@redbmk
Copy link
Author

redbmk commented May 13, 2024

Not sure if it would be an issue in the test runners but I noticed if I run the linter and then fixer tests I get an error. I think if it runs fixers first then linters, it wouldn't show up, but the fix I just pushed lets it work either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Biome] Linter not usable
3 participants