-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support file formatting and linting #38
Conversation
@@ -1,8 +1,14 @@ | |||
mod common; | |||
use common::*; | |||
use smir_pretty::{stable_mir_driver, print_all_items_verbose}; | |||
use smir_pretty::{print_all_items_verbose, stable_mir_driver}; |
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 this is actually dead code? Cannot find any print_all_items_verbose
🤔
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 will add an issue to go through and see if we can clean up old comments and unused imports!
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. Massive diff, I trust it is all about formatting... (tests are passing ✔️ )
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. Just one comment.
.github/workflows/test.yml
Outdated
run: | # Warning check should be redundant | ||
RUSTFLAGS='--deny warnings' cargo build -vv |
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.
What does this comment mean? Maybe it would make more sense to put --deny warnings
in the build step for code-quality
?
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 is, I think that cargo clippy
should catch every error the compiler would have.It is redundant, should I just remove it?
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 made me realise that the integration-tests
job wasn't dependent on code-quality
. I think we normally do that and so I have changed it
Yeah it is all formatting changes |
This PR will add code quality checks to CI through cargo clippy and cargo fmt
Closes #37