-
Notifications
You must be signed in to change notification settings - Fork 18
Add rustfmt to CI to ensure formatting is checked
#66
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
Conversation
|
👋 I see @tankyleo was un-assigned. |
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
tankyleo
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.
LGTM, reproduced on my machine, two questions
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Check formatting on Rust ${{ matrix.toolchain }} | ||
| if: matrix.check-fmt |
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.
When is this true ?
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.
Ah, lol, good catch. It would be if I checked in the change. For simplicity I now simply dropped the line.
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 would be if I checked in the change.
Can you clarify this ? Thank you :)
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 am also unclear how ${{ matrix.toolchain }} gets defined, thank you for the clarification.
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.
🤦♂️ Sorry, that variable is likely only available if you use a matrix strategy in the workflow, see https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/run-job-variations
Now dropped it.
It would be if I checked in the change.
Can you clarify this ? Thank you :)
Yes, I previously made/copy pasted some changes that weren't fully making it here. However, for now it's probably better to keep it simple anyways.
0867dac to
5d714c8
Compare
.. we simply add a `cargo fmt` step to our Rust CI
5d714c8 to
b6b3ff6
Compare
.. we simply add a
cargo fmtstep to our Rust CI