-
Notifications
You must be signed in to change notification settings - Fork 4
Fix CI and clippy issues #10
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
WanzenBug
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.
Thanks for the contribution! I don't want to bump the MSRV if we don't have to, so I'd rahter fix the CI workflow than bumping it.
g2gen/Cargo.toml
Outdated
| categories = [ "no-std", "algorithms" ] | ||
| keywords = [ "finite-field", "galois", "macro", "newtype"] | ||
| rust-version = "1.61" | ||
| rust-version = "1.70" |
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.
cargo +1.61.0 check
Runs just fine. Issue seems to be running cargo +1.61.0 test, which I'd say can be ignored. I guess that means the CI setup should be updated so it only runs check in the MSRV case.
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.
In that case, I think we need to pin dependencies to an older version, cause I get
error: package
textwrap v0.16.2cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.61.0
when trying to run the tests with cargo +1.61.0 test
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.
Yes. the dev-dependecies are no longer compatible with 1.61.0. So you cannot run cargo +1.61.0 test. But you can still run cargo +1.61.0 check. So g2p will run just fine with the older rust version, it's just that we can't test it with the current tests :/
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 added a Cargo.lock file, with dependencies set to versions that are compatible with 1.61. With this the tests should work fine when run with cargo +1.61.0 test
WanzenBug
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!
Thank you!
This PR does the following:
EqandPartialEqand instead derives them (similar toHash) to avoid this Clippy warning