dense-analysis / ale

Check syntax in Vim/Neovim asynchronously and fix files, with Language Server Protocol (LSP) support
BSD 2-Clause "Simplified" License
13.38k stars 1.42k forks source link

fix(biome): run all enabled biome fixers #4763

Closed redbmk closed 1 week ago

redbmk commented 2 months ago

fixes: #4754

redbmk commented 2 months ago

@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 commented 1 month ago

@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 commented 1 month ago

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

hsanson commented 1 month ago

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 commented 1 month ago

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

redbmk commented 1 month ago

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.

Spixmaster commented 1 week ago

What is the issue with the commits? Why do not the tests finish?

I would like to inspect the output but can only call the one of "continuous-integration/appveyor/pr".

Edit: I just saw the comment of @redbmk, https://github.com/dense-analysis/ale/pull/4775#issuecomment-2180645822.

hsanson commented 1 week ago

@redbmk if you could fix the conflict I can merge this.

redbmk commented 1 week ago

LGTM, thanks

looks like I need to fix merge conflicts with the other one now. I'll try to do that asap

redbmk commented 1 week ago

@hsanson should be good now