fix: resolve compiler warnings and add CI warning detection#76
fix: resolve compiler warnings and add CI warning detection#76
Conversation
Fix unused imports in objectives, entry_requirement, prize, and ticket_booth. Handle #[must_use] return in entry_fee mock. Intentionally leave one unused import (Fixed in math/tests.cairo) to verify CI catches warnings. Will be fixed in follow-up push. Add warning detection to both CI workflows: build and test steps now fail when source-located compiler warnings are found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances code quality and maintainability by eliminating existing compiler warnings and establishing a robust CI process to prevent future regressions. By making CI builds fail on warnings, it ensures that all new code adheres to strict quality standards, improving the overall reliability and readability of the codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR changes CI workflows to capture build and test output and fail on any detected Cairo compiler warnings, and makes small code cleanups in several Cairo files (removing unused imports and discarding an unused return value in a test mock). Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions Runner"
participant Build as "Workspace Build (cairo-compile)"
participant Tests as "Test Runner"
participant Log as "Log File (/tmp/test.log)"
participant Scanner as "CI Warning Scanner"
GH->>Build: run build, stream output -> /tmp/build.log
Build-->>GH: success/failure + stdout/stderr
GH->>Tests: run tests, pipe output -> /tmp/test.log
Tests-->>Log: write combined stdout/stderr
GH->>Scanner: scan /tmp/build.log and /tmp/test.log for Cairo warnings
alt warnings found
Scanner-->>GH: print warning context, fail job
else no warnings
Scanner-->>GH: report clean, continue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on improving code quality by resolving compiler warnings, primarily related to unused imports and unhandled #[must_use] return values. The changes are correct and achieve the goal of cleaning up the codebase. I have one suggestion to improve a test mock by propagating a return value instead of just ignoring it, which would make the mock more useful for testing.
| #[external(v0)] | ||
| fn set_entry_fee(ref self: ContractState, context_id: u64, entry_fee: EntryFee) { | ||
| self.entry_fee.set_entry_fee(context_id, entry_fee); | ||
| let _ = self.entry_fee.set_entry_fee(context_id, entry_fee); | ||
| } |
There was a problem hiding this comment.
While let _ = ... silences the compiler warning, it's generally better to propagate #[must_use] return values, especially in a test mock. The underlying set_entry_fee function returns an Option<EntryFeeConfig>, which could be valuable for assertions in your tests.
By modifying the function to return this value, the mock becomes more transparent and robust. You'll also need to add EntryFeeConfig to your imports from crate::entry_fee::structs.
#[external(v0)]
fn set_entry_fee(ref self: ContractState, context_id: u64, entry_fee: EntryFee) -> Option<EntryFeeConfig> {
self.entry_fee.set_entry_fee(context_id, entry_fee)
}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR cleans up Cairo compiler warnings across the metagame and interfaces packages, and tightens CI so that source-located compiler warnings cause workflow failures. This supports the repo’s goal of keeping smart-contract builds/test runs warning-free and preventing warning regressions from landing on main.
Changes:
- Remove unused imports and address
#[must_use]return values to eliminate existing compiler warnings. - Update
pr-ci.ymlandmain-ci.ymlto detect (and fail on) compiler warnings duringscarb buildandsnforge test.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/metagame/src/ticket_booth/ticket_booth_component.cairo | Removes unused struct imports from the ticket booth component. |
| packages/metagame/src/prize/structs.cairo | Removes an unused test-only import. |
| packages/metagame/src/entry_requirement/entry_requirement_component.cairo | Removes unused extension dispatcher imports and an unused struct import. |
| packages/metagame/src/entry_fee/tests/mocks/entry_fee_mock.cairo | Explicitly discards a #[must_use] return value to silence warnings in the mock. |
| packages/interfaces/src/minigame/objectives.cairo | Removes an unused import from the minigame objectives interface. |
| .github/workflows/pr-ci.yml | Adds build/test log capture and warning detection to fail PR CI on compiler warnings. |
| .github/workflows/main-ci.yml | Adds build/test log capture and warning detection to fail main CI on compiler warnings. |
| if: always() && steps.cache-tools.outcome != 'failure' | ||
| run: | | ||
| if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then | ||
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | ||
| grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log |
There was a problem hiding this comment.
The warning detector is keyed only on source-location lines (--> *.cairo:line:col), which are also emitted for compilation errors. Because this step runs with if: always(), a compile failure can be misreported as “warnings” (and potentially add noise on top of the real failure). Consider narrowing the grep to actual warning headers (e.g., match warning: and then include the following location lines), or gate this step to only run when the test command succeeded.
| if: always() && steps.cache-tools.outcome != 'failure' | |
| run: | | |
| if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then | |
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | |
| grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log | |
| if: success() && steps.cache-tools.outcome != 'failure' | |
| run: | | |
| if [ -f /tmp/test.log ] && grep -q '^warning:' /tmp/test.log; then | |
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | |
| grep -A1 -E '^warning:' /tmp/test.log |
| if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then | ||
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | ||
| grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log |
There was a problem hiding this comment.
The warning detector is keyed only on source-location lines (--> *.cairo:line:col), which are also emitted for compilation errors. Because this step runs with if: always(), a compile failure can be misreported as “warnings” (and potentially add noise on top of the real failure). Consider narrowing the grep to actual warning headers (e.g., match warning: and then include the following location lines), or gate this step to only run when the test command succeeded.
| if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then | |
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | |
| grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log | |
| if [ -f /tmp/test.log ] && grep -q 'warning:' /tmp/test.log; then | |
| echo "::error::Test compilation produced warnings. Fix all warnings before merging." | |
| grep -n -B2 -A2 'warning:' /tmp/test.log |
CI correctly detected the intentional warning — remove it now. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
#[must_use])pr-ci.ymlandmain-ci.yml— build and test steps now fail on source-located compiler warningsFixedinmath/tests.cairo) to verify CI catches it — will be fixed in follow-up pushTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit