Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

accounts: clippy and style fixes#11443

Open
vorot93 wants to merge 8 commits into
openethereum:masterfrom
vorot93:accounts-clippy
Open

accounts: clippy and style fixes#11443
vorot93 wants to merge 8 commits into
openethereum:masterfrom
vorot93:accounts-clippy

Conversation

@vorot93

@vorot93 vorot93 commented Feb 3, 2020

Copy link
Copy Markdown

This is the initial batch of style fixes as suggested by clippy as well as the general patterns of modern Rust. A couple of notes:

  • This is based on all possible clippy hints including pedantic and nursery ones. I assembled a uniform blacklist of common lint occurrences that cannot (easily) be fixed which would be the same for all crates in the repo. This would ease its maintenance in the future.
  • Besides the fixes suggested by clippy, I also removed the match by reference semantics in favor of explicit pre-match borrowing (or as_ref helper method).

@parity-cla-bot

Copy link
Copy Markdown

It looks like @vorot93 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Comment thread accounts/ethkey/cli/src/main.rs Outdated
Comment thread accounts/ethkey/cli/src/main.rs Outdated
Comment thread accounts/ethkey/cli/src/main.rs Outdated
Comment thread accounts/ethkey/cli/src/main.rs Outdated
vorot93 and others added 3 commits February 4, 2020 12:25
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Comment thread accounts/ethkey/src/lib.rs
@vorot93 vorot93 requested a review from niklasad1 February 4, 2020 16:23
Comment thread accounts/ethstore/src/account/safe_account.rs Outdated
Comment thread accounts/ethstore/src/accounts_dir/disk.rs Outdated
dst.insert(a)?;
Ok(address)
}).collect()
let mut out = Vec::with_capacity(accounts.len());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess that technically the lint triggered this was map().filter() instead map_filter()?

Then you switch to a for loop instead because it was not possible to return the error in filter_map?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I simply didn't want to untangle the mix of Option and Result. And in general the imperative approach seems to me much clearer than the map-and-collect of results. It also returns earlier (on first error) although I'm not sure if it's the desired behavior here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

right, it seems that the actual implementation of KeyDirectory::insert always returns Ok but I don't know, let's ignore it for this PR.

iv.copy_from_slice(&wallet.encseed[..16]);

let mut ciphertext = vec![];
let mut ciphertext = Vec::new();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not a clippy-lint, right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it was not, I can drop this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is fine for me, I like Vec::new better but let's keep it open so the others can see this too.

let mut out = Vec::with_capacity(accounts.len());
for account in accounts {
let address = account.address;
if existing_accounts.contains(&address) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

to have the same behavior as before

Suggested change
if existing_accounts.contains(&address) {
if !existing_accounts.contains(&address) {

@sorpaas sorpaas added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants