feat(js-api): support applying lint fixes #1956
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mnahkies
changed the title
| path, | ||
| categories: ["Syntax", "Lint"], | ||
| max_diagnostics: Number.MAX_SAFE_INTEGER, | ||
| }); | ||
|
|
||
| const hasErrors = diagnostics.some( |
There was a problem hiding this comment.
I'm unsure about this part - it would be great to get some guidance from someone more familiar with the biome code base here
There was a problem hiding this comment.
Fixing a file, actually, works backwords: first you fix the file, then you pull the diagnostics.
Why? Because when fixing a file, you actually end reducing the diagnostics by applying the fixes. This means we need to pull the diagnostics after we fixed the file, to show the users any remaining diagnostics.
That's what we do in the CLI:
There was a problem hiding this comment.
That makes perfect sense, I'll go ahead and make that change.
I'm surprised we don't need to do the same in formatContent above? Though a quick test I see that I get back an [] for diagnostics - does the formatter simply not emit warning / information diagnostics on unformatted files?
There was a problem hiding this comment.
Formatting has different expectations and requirements.
Usually, formatters can't format broken syntax, that's why we need to check for parsing errors before running the formatter.
Same goes for broken syntax in Biome, broken blocks are kept verbatim, only blocks that are synaptically correct.
Given the previous statement, it's safe to assume that formatters don't emit new syntax/parse diagnostics, because there weren't any before. If they do, that's a bug in the formatter.
There was a problem hiding this comment.
I guess I naively would've expected that pulling diagnostics prior to formatting the file would've come back with the details of where it wasn't formatted correctly, and that after these issues would be gone (leaving only issues that couldn't be fixed from syntax errors, etc) - though I didn't consider that we'd just skip formatting in the presence of syntax errors, presumably to avoid making the errors worse.
There was a problem hiding this comment.
(fixed in 139c3dc)
| path, | ||
| categories: ["Syntax", "Lint"], | ||
| max_diagnostics: Number.MAX_SAFE_INTEGER, | ||
| }); | ||
|
|
||
| const hasErrors = diagnostics.some( |
There was a problem hiding this comment.
Fixing a file, actually, works backwords: first you fix the file, then you pull the diagnostics.
Why? Because when fixing a file, you actually end reducing the diagnostics by applying the fixes. This means we need to pull the diagnostics after we fixed the file, to show the users any remaining diagnostics.
That's what we do in the CLI:
| it("should fix lint issues and return new content if fileFileMode is SafeAndUnsafeFixes", () => { | ||
| const result = biome.lintContent('let a = "foo " + Date.now() + " bar"', { | ||
| filePath: "example.js", | ||
| fixFileMode: "SafeAndUnsafeFixes", | ||
| }); | ||
|
|
||
| expect(result.content).toMatchSnapshot("fixed content"); | ||
| }); |
There was a problem hiding this comment.
We should add another test. A content that contains code that triggers a safe and unsafe fix, and then attempt to fix only the safe fixes.
Then, should assert that:
- we fixed only the safe fix
- there's still a diagnostic about the the rule that triggered the unsafe fix
|
Requested some changes because the logic of fixing a file should work the other way around:
|
mnahkies
force-pushed
the
mn/feat/support-applying-lint-js-api
branch
from
7ed1421 to
6793ae0
Compare
| const result = biome.lintContent(inputCode, { | ||
| filePath: "example.js", | ||
| }); | ||
| expect(result.diagnostics).toMatchObject([ |
There was a problem hiding this comment.
I've changed the assertions on diagnostics to just check the category instead of using snapshots - I felt this was more maintainable, but happy to change back if you disagree
mnahkies
force-pushed
the
mn/feat/support-applying-lint-js-api
branch
from
6793ae0 to
139c3dc
Compare
|
|
||
| return { content: code }; | ||
| }) | ||
| : { content }; |
There was a problem hiding this comment.
Is there a reason why we create a new object, instead of just returning content?
Summary
I've recently switched to using
biomeprogrammatically to format code before writing it to disk it in a code generation projectI was experimenting with the idea of having it additionally lint the files, but noticed there was no way to apply lint fixes without resorting to using private API's currently.
This aims to allow applying the fixes, following a similar API to that of
formatContentNote
I've not tried to implement a "fix range" option - as far as I can see that isn't something that is directly supported by the underlying analyzer.
It might be possible to implement using
code_actionsand filtering for those with asuggestionconstrained byspan? Probably more work than I have time to attempt at the moment though.Test Plan
Unit tests demonstrate that the change is backwards compatible, and works.