-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore: improve setup & CI config #6
Conversation
* ignore snapshot files for linting * move CI-specific options
de46eb6
to
d53db78
Compare
Co-authored-by: Jordan Harband <[email protected]>
"**/*.fixture.mjs", | ||
"**/*.fixture.mts", | ||
"**/*.fixture.js", | ||
"**/*.fixture.ts", | ||
"**/*.mock.mjs", | ||
"**/*.e2e.mjs", | ||
"**/*.e2e.mts", | ||
"**/*.e2e.js", | ||
"**/*.e2e.ts", | ||
"**/*.spec.mjs", | ||
"**/*.spec.mts", | ||
"**/*.spec.js", | ||
"**/*.spec.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super verbose, and the tsconfig docs specifically say they support full globs, but using {mjs,mts,js,ts}
silently fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
instead of requesting review maybe add code owner ? |
f152431
to
d47e11e
Compare
I solved the CLI arg issue 🥳 resolved CI issueHowever, I'm getting a seemingly erroneous failure in CI:
The following errors are expected (because there are no files yet):
I created a local branch cloned from this one, rebased #7 on top, and manually ran the command, and it works as expected. I think this is ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we set options for use-block-statements? (I think we should use "multi")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think we should use "multi")
It's sad but I don't see support for an equivalent to multi
. But I don't find it a problem personally, because I think it's fine to force the {...}
block whatever the condition.
.github/workflows/ci.yml
Outdated
cache: 'npm' | ||
- run: npm ci | ||
- run: node --run lint | ||
- run: node --run format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use prebuilt GA by biome because it's include a reporter so on PR we see where there are something wrong.
https://biomejs.dev/recipes/continuous-integration/#github-actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 That would make CI and local behave differently. I think we should avoid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's run command "biome ci ./".
Eh wait, this ins't on scripts
other things can be added
@AugustinMauroy ah, it looks like biome chose to specifically disallow configuring certain options and unfortunately chose objectively undesirable ones, such as forcefully collapsing line breaks (their choice bloats diffs and makes it more difficult to see what was actually changed). I despised that about prettier, Edit: actually, I think the undesirable stuff is isolated to biome's formatter. Perhaps we can just disable that; I'm not sure of what all it does though—the docs don't appear to exhaustively list what it does. |
--workspaces