-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
Conflicts: rpc/src/v1/types/bytes.rs
Conflicts: parity/signer.rs signer/Cargo.toml signer/src/lib.rs signer/src/ws_server/mod.rs
Conflicts: dapps/src/lib.rs parity/main.rs parity/signer.rs signer/Cargo.toml signer/src/lib.rs
handler: Arc<IoHandler>, | ||
} | ||
|
||
impl Default for ServerBuilder { |
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.
is this default impl used?
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.
Nope, but it's recommended by rust-clippy to have one if you have a no-argument pub fn new() -> Self
.
I prefer to use ServerBuilder::new()
then ServerBuilder::default()
, but like to keep clippy happy as well :)
Do you think we should just use default()
?
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.
ah, i didn't realize that it was for clippy's benefit. I guess intuitively i would probably have it the other way -- have new()
call default()
rather than the other way around, but that's definitely just a personal preference.
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.
think there's something to be said for making default
the canonical means of naming/refering to parameter-less construction functions; it's got a stronger semantic notation given it's a trait rather than Just Another Consistently Named Function.
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.
Default implementation will be removed in future PRs anyway.
No description provided.