-
Notifications
You must be signed in to change notification settings - Fork 89
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
WIP: feat: switch to biome formatter #487
base: main
Are you sure you want to change the base?
Conversation
No, like with other formatters we should just skip formatting if
Yes.
No, we should not support both. The main purpose of formatters is for simplifying our snapshot tests, not necessarily for users own idiosyncratic formatting choices. |
Code looks goodish but tests aren't passing - you probably need to update the GitHub actions to install Biome. |
I added biome to the actions but this appears to require your approval. Do
you want me run these as a PR in my own fork so you avoid using your runner
credits?
…On Fri, Sep 6, 2024, 4:51 PM Morgante Pell ***@***.***> wrote:
Code looks goodish but tests aren't passing - you probably need to update
the GitHub actions to install Biome.
—
Reply to this email directly, view it on GitHub
<#487 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEFKCQMQC3XI72CPAAABLZVIIVPAVCNFSM6AAAAABNW7D3POVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZUG44TEMBSGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I don't mind clicking approve, but you should also be able to run tests locally. It looks like some snapshots will need updating, and the standard library tests will also need Biome installed. |
Use self hosted runner
I got the snapshot tests fixed in the gritql repo. I love those snapshot tests. As a quick aside, I wasn't sure if there is a recommended way to use my local development version of grit/marzano, so I just added it to my path. If this is dumb for some reason, please let me know, but it appears to use the new biome formatter. $ cd Projects/grit.io/stdlib When I ran the stdlib tests, I see a lot of changes. At first glance they appear to be mostly quote issues (single quote to double quote) and missing semicolon issues, for the most part. I see two obvious paths: configure biome to match prettier defaults, or fix the tests to match biome preferences. Do you have an opinion on this? There are some other formatting changes in the react/TSX code so this diff might get big regardless, or the biome config might get big, depending on which way we go. Here is a gist with the stdlib test results: https://gist.github.com/xrd/aafb4eb68209e553a6c8c79abe12dae6 |
Biome should actually match prettier defaults. From the diff, it looks like you're not actually successfully running the formatter at all. Ex. this code is clearly unformatted:
|
Use biome formatter instead of prettier for JS,TS, TSX and JSON .
For formats which are not supported by biome (like Markdown, YAML, CSS, HTML: https://biomejs.dev/internals/language-support/) continue to use prettier.
(re: #168)