Skip to content
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

Add linter to CI and applied some fixes #35

Merged
merged 6 commits into from
Dec 5, 2023
Merged

Add linter to CI and applied some fixes #35

merged 6 commits into from
Dec 5, 2023

Conversation

andresuribe87
Copy link
Contributor

This PR adds the clippy linter to the repo. See https://doc.rust-lang.org/clippy/index.html for more information.

I applied the automatic fixes. There's still one more error: https://rust-lang.github.io/rust-clippy/master/index.html#/module_inception, which we need to decide how to handle. Open for feedback on how to best deal with it.

Copy link
Contributor

@amika-sq amika-sq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Few minor comments:

I applied the automatic fixes. There's still one more error: https://rust-lang.github.io/rust-clippy/master/index.html#/module_inception, which we need to decide how to handle. Open for feedback on how to best deal with it.

I can take this one on and fix it. We consolidated on the module naming structure here: TBD54566975/tbdex-rs#31 (comment)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@amika-sq amika-sq linked an issue Dec 4, 2023 that may be closed by this pull request
amika-sq and others added 2 commits December 4, 2023 13:09
This error resulted from the fact that we had a files that matched their folder's name.
E.g: We had crypto/src/key/key.rs. The module's name is `key`, and it contained a module named `key` within it.

To fix, I removed all of this inception. `key.rs`'s code now lives in `mod.rs`, same with `key_manager.rs`. Subsequently, this allows us to r
emove all of the global use statements like `pub use key::*;`!

This also brings consistency with the approach taken in the `tbdex-rs` repo, as discussed here: TBD54566975/tbdex-rs#31 (comment)
Co-authored-by: Adam Mika <88001738+amika-sq@users.noreply.github.com>
@KendallWeihe
Copy link
Contributor

cc @leordev I don't think this impacts your work, but making you aware of the use of clippy here

@@ -1,8 +1,25 @@
mod key;
pub use key::*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I prefer the explicit syntax for imports

@@ -39,7 +38,7 @@ mod tests {
let signature = private_key.sign(payload).unwrap();

let public_key = private_key.to_public();
let verification_warnings = public_key.verify(&payload, &signature).unwrap();
let verification_warnings = public_key.verify(payload, &signature).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What provoked this change? Why is signature passed by reference but payload isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this was the result of running cargo clippy --fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, because payload was already initialized by reference whereas signature was not

@andresuribe87 andresuribe87 merged commit b1f36ad into main Dec 5, 2023
1 of 9 checks passed
@andresuribe87 andresuribe87 deleted the clippy branch December 5, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add clippy linter
3 participants