chore: Justfile#14
Conversation
There was a problem hiding this comment.
I know that in the previous iteration of this PR you were suggested to use a Justfile, but since we have package.json already in use and most of these commands are one-liners, we can use package.json scripts instead, right?
If a command is longer than one line or would be better on multiple lines, a script can be added like in a scripts/ directory and called by pnpm.
There was a problem hiding this comment.
Fair point, but in case of some longer commands (like snapshots, Solhint etc) it's easier to just write just <target> instead of remembering the full command.
There was a problem hiding this comment.
It's a bit annoying to use NPM scripts here because package.json is in dev and so it would be something like npm run --prefix dev whatever.
The Just file looks more easy to work with than NPM scripts so I'm weakly in favor of keeping the new tool. But I'm also fine with dropping it for simplicity if we decide that.
There was a problem hiding this comment.
cool that it even works my <tab> autocomplete too
|
|
||
| # Print coverage summary (excludes `test`/`script` files) | ||
| coverage-summary: | ||
| forge coverage --no-match-coverage "(test|script)" --report summary |
There was a problem hiding this comment.
As mention here, the regex matches too much, we should match based on the start of the path.
There was a problem hiding this comment.
Fixed by adding "^" at the start to only filter the start of the path.
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Nice! Just small comments.
| { | ||
| "increment - success": "43476" | ||
| } No newline at end of file |
There was a problem hiding this comment.
How was this file generated? I removed it and tried to recreate it with any commands from just and it didn't work.
There was a problem hiding this comment.
My bad, I've removed a vm.snapshotGasLastCall from a test file by mistake. If you run just snapshot now, it will be present.
There was a problem hiding this comment.
It's a bit annoying to use NPM scripts here because package.json is in dev and so it would be something like npm run --prefix dev whatever.
The Just file looks more easy to work with than NPM scripts so I'm weakly in favor of keeping the new tool. But I'm also fine with dropping it for simplicity if we decide that.
|
|
||
| # Run tests with `forge test` | ||
| test: | ||
| forge test --force --isolate -vvv --show-progress --gas-snapshot-check true |
There was a problem hiding this comment.
I really think keeping --force flag in these commands is helpful to avoid caching issues, but I'm fine with removing it and simply running build-all if they happen.
| forge snapshot --force --isolate --desc --show-progress | ||
|
|
||
| # Run build, lint, slither, test, coverage-check, snapshot | ||
| all: |
There was a problem hiding this comment.
why not using Just dependencies instesad of imperative form?
There was a problem hiding this comment.
Good call, didn't use Justfile much before and was used to Makefile syntax
4 participants
Description
Add
justcommands for build, format, lint, Slither, tests, coverage, snapshots, and all checks.Context
This PR depends on the previous PRs. It is only fully testable after the Foundry config, local dependencies, Solhint, and Slither PRs are merged.
Out of Scope
This PR does not add CI or git hooks.
Testing Instructions
just help.just build.just lint.just slither.just test.just coverage-check.just snapshot.just all.