Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Migrate to rust 2018 edition #138

Merged
merged 11 commits into from
Dec 11, 2018
Merged

Migrate to rust 2018 edition #138

merged 11 commits into from
Dec 11, 2018

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Dec 10, 2018

 - apply cargo fix --edition
 - probably can remove most `extern crate foo;`
use commands::KmsCommand;
use config::KmsConfig;
use crate::commands::KmsCommand;
use crate::config::KmsConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how much you care it might be nice to collapse these into nested imports (I've been doing that in my own crates), e.g.

use crate::{
    commands::KmsCommand,
    config::KmsConfig
};

 - use dyn keyword
 - use crate:: (absolute path)
 - anonymous lifetimes
 - inclusive ranges via `..=`
 - remove some extern crate
@@ -12,6 +12,7 @@ impl<IoHandler> UnixConnection<IoHandler>
where
IoHandler: io::Read + io::Write + Send + Sync,
{
#[allow(clippy::new_ret_no_self)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lots of fun with these... I have mostly been adding an allow for these as well, although sometimes there are better solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this could be a impl From<IoHandler> for UnixConnection<IoHandler> instead of new. On the other hand we do not err in any case and could also change the method below to:

Suggested change
#[allow(clippy::new_ret_no_self)]
/// Create a new `UnixConnection` for the given socket
pub fn new(socket: IoHandler) -> Self {
Self { socket }
}

@liamsi liamsi changed the title WIP: switch to rust 2018 edition Migrate to rust 2018 edition Dec 11, 2018
@@ -21,6 +21,7 @@ pub const MAX_LENGTH: usize = 50;
pub struct Id([u8; MAX_LENGTH]);

impl Id {
#[allow(clippy::new_ret_no_self)]
Copy link
Contributor

@tarcieri tarcieri Dec 11, 2018

Choose a reason for hiding this comment

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

Alternatively this could be impl FromStr for Id.

Edit: oh hey wait, that already exists! 😂 https://github.com/tendermint/kms/blob/4983f3b2426a783bd7d3c1ea9b4ea3dce1080f79/tendermint-rs/src/chain/id.rs#L80

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could still remove pub fn new and place the logic into from_str?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

);
process::exit(1);
},
Some(ref key_type) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe combine this with the if below?

Some(ref key_type) if key_type != DEFAULT_KEY_TYPE => {

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively the match could be replaced with:

if let Some(key_type) = self.key_type.as_ref() {

...in which case you'd still need the nested ifs, but you can get rid of the None below

Copy link
Contributor

@tony-iqlusion tony-iqlusion left a comment

Choose a reason for hiding this comment

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

Ugh GitHub refresh not keeping up with your changes. I'll call this close enough 😉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants