feat: multi file lint rule codegen support#3052
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
d4f75fc to
9b63a72
Compare
ematipico
left a comment
There was a problem hiding this comment.
I left a few questions, but overall it's looking amazing!
| if let Some(file_path) = &test.file_path { | ||
| normalize_file_path(file_path) | ||
| } else { | ||
| format!("code-block.{}", test.tag) |
There was a problem hiding this comment.
We don't validate if the file has or doesn't have an extension. Do we check for that in the main repository ?
There was a problem hiding this comment.
yeah that isn't in the main repro but is a good idea, I can add in a follow up both here and the other repo if you would like
| /// file system to a markdown section. This allows lint rules to access | ||
| /// multiple related files within the same documentation section while | ||
| /// keeping different examples isolated. | ||
| fn parse_file_system(docs: &'static str) -> Result<HashMap<usize, HashMap<String, String>>> { |
There was a problem hiding this comment.
It's not very clear what a "section" is. You might want to clarify that.
Also, it would be great if we explain the business logic of the parsing somewhere
There was a problem hiding this comment.
I tried to improve the documentation to be more clear. Let me know if you think it's better or could be improved more
| <pre class="language-text"><code class="language-text"><a href="file:///async-fn.ts">/async-fn.ts</a>:4:1 <a href="https://biomejs.dev/linter/rules/no-floating-promises">lint/nursery/noFloatingPromises</a> ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━<br /><br /> <strong><span style="color: lightgreen;">ℹ</span></strong> <span style="color: lightgreen;">A "floating" Promise was found, meaning it is not properly handled and could lead to ignored errors or unexpected behavior.</span><br /> <br /> <strong>2 │ </strong> return 'value';<br /> <strong>3 │ </strong>}<br /> <strong><span style="color: Tomato;">></span></strong> <strong>4 │ </strong>returnsPromise().then(() => {});<br /> <strong> │ </strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><strong><span style="color: Tomato;">^</span></strong><br /> <strong>5 │ </strong><br /> <br /> <strong><span style="color: lightgreen;">ℹ</span></strong> <span style="color: lightgreen;">This happens when a Promise is not awaited, lacks a `.catch` or `.then` rejection handler, or is not explicitly ignored using the `void` operator.</span><br /> <br /></code></pre> | ||
|
|
||
| ```file=async-fn2.ts | ||
| ```ts title='async-fn2.ts' |
arendjr
left a comment
There was a problem hiding this comment.
Great work!
Indeed it would be nice if you could expose get_test_services() from biome_test_utils so we don't need to duplicate it here, but it's fine if you do that in a separate PR 👍
b5e72fe to
0af1dbf
Compare
|
After reading the business logic and looking at the final result https://deploy-preview-3052--biomejs.netlify.app/linter/rules/no-import-cycles/#invalid , I have the following question: do we need headings for each file? They seem redundant if you look at the web page, because the file name are already part of code block |
|
@ematipico I opened a small clean up PR here that would remove the filenames in favor of the |
ematipico
left a comment
There was a problem hiding this comment.
Looks good! Happy to merge after the conflicts have been resolved
0af1dbf to
fa45f16
Compare
|
@ematipico merge conflicts resolved! |
Summary
This PR modifies the website codgen process to utilize the
file=<path>code block annotation for setting rendered block filenames as suggested in this comment and also adds support for rendering multi-file lint rule diagnostics. Much of the functionality is similar to this PR but ported to this repo.The approach here is a bit different from the one in the main Biome repo, opting instead run a second parse pass to collect all files for the file system if we detect the need for it. This seemed like the better approach here since with the codegen process we are dealing with writers that write while we parse making the deferred evaluation approach used in the other PR much less practical or performant. File system parsing is run lazily though only when we detect the need for it so should hopefully add very little extra overhead. I'm open to exploring other approaches if there are any suggestions though.
@arendjr I mostly just copied over the
get_test_servicesfunction over from the other repo with your changes as well. Maybe it would make sense to add this to thebiome_test_utilspackage in the main repo? If it would be helpful I could do so in a follow up PR if you would like.Example: