-
Notifications
You must be signed in to change notification settings - Fork 41
chore: add existing checks to pre-commit, use pre-commit in CI #52
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
base: graphite-base/52
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d5644bf to
bc32940
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 95.43% 95.60% +0.16%
==========================================
Files 45 45
Lines 9446 9783 +337
==========================================
+ Hits 9015 9353 +338
+ Misses 431 430 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bc32940 to
04e5d6b
Compare
noa-starkware
left a comment
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware)
.pre-commit-config.yaml line 6 at r2 (raw file):
- id: cairo-scarb-fmt name: check cairo code is formatted entry: scarb fmt -w --check
Is this going to slow the push?
arad-starkware
left a comment
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware and @noa-starkware)
.pre-commit-config.yaml line 6 at r2 (raw file):
Previously, noa-starkware wrote…
Is this going to slow the push?
Not the push, but the commit: yea, a bit.
noa-starkware
left a comment
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @arad-starkware and @NirLevi-starkware)
.pre-commit-config.yaml line 6 at r2 (raw file):
Previously, arad-starkware wrote…
Not the push, but the commit: yea, a bit.
Lets try it for now and maybe remove it later
noa-starkware
left a comment
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arad-starkware and @NirLevi-starkware)
.pre-commit-config.yaml line 12 at r2 (raw file):
- id: ensure-flow-struct-tests name: ensure flow structs have tests entry: scripts/check_all_flows_have_test.py --allow-duplicate-tests
Is this necessary here?
arad-starkware
left a comment
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware and @noa-starkware)
.pre-commit-config.yaml line 12 at r2 (raw file):
Previously, noa-starkware wrote…
Is this necessary here?
I wouldn't say any of these checks (in this PR) are necessary, since they're checked in the CI. But I think it's better to find this out here instead of only after pushing.
9c47c2d to
5a66e7e
Compare
5a66e7e to
4baaed7
Compare
4baaed7 to
aaee246
Compare
894eed0 to
c44bef8
Compare
aaee246 to
ad14015
Compare
c44bef8 to
411f3b9
Compare
ad14015 to
2fcf1b8
Compare
2fcf1b8 to
8bbad41
Compare
8bbad41 to
e989adb
Compare
| - uses: software-mansion/setup-scarb@v1 | ||
| with: | ||
| scarb-version: "2.12.2" | ||
| - uses: pre-commit/[email protected] |
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.
Bug: Python setup removed but hooks need Python
The pre-commit job removed the explicit Python setup that was in the old verify_flows job, but the pre-commit hooks still run Python scripts (check_all_flows_have_test.py and check_scarb_url_lock.py configured with language: system). While ubuntu-latest has Python pre-installed, relying on it without explicit setup is fragile and could break if GitHub modifies their runner images.
4348046 to
b5cde98
Compare
.pre-commit-config.yaml
Outdated
| entry: scarb fmt -w --check | ||
| language: system | ||
| pass_filenames: false | ||
| stages: [pre-push] |
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.
Bug: Pre-push hooks won't run in CI
The cairo-scarb-fmt and ensure-flow-struct-tests hooks are configured with stages: [pre-push], but the CI job uses pre-commit/action which runs pre-commit run --all-files by default. This command only executes hooks in the commit stage, so these two important checks that previously ran in CI will no longer execute. The workflow needs to specify --hook-stage push or --hook-stage manual, or the hooks should use stages: [commit] or stages: [manual] to run in CI.
Additional Locations (1)
b5cde98 to
f467ee6
Compare
f467ee6 to
4a2bac2
Compare

This change is
Note
Run pre-commit in CI, add hooks for flow tests and Cairo formatting, and remove redundant Python/setup and formatting steps from the workflow.
pre-commitjob running withsetup-snfoundry,setup-scarb, andpre-commit/action(withSKIP=scarb-toml-url-lock).scarb fmt --checkfromtestjob.ensure-flow-struct-tests(runsscripts/check_all_flows_have_test.py --allow-duplicate-tests).cairo-scarb-fmt(scarb fmt -w --check).scarb-toml-url-lock; configure stages (pre-commit,pre-push,manual).scripts/check_all_flows_have_test.pyexecutable via shebang.Written by Cursor Bugbot for commit 4a2bac2. This will update automatically on new commits. Configure here.