chore: clippy#1011
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| PoolUnhealthy, | ||
|
|
||
| #[error("checked in untracked connection: {0}")] | ||
| UntrackedConnCheckin(BackendKeyData), |
There was a problem hiding this comment.
This seems fine if we don't need it, but I really don't think it'd be a problem to do Box<BackendKeyData> either
There was a problem hiding this comment.
This was probably to preserve the Copy derive on the Error struct.
|
|
||
| let server = Box::new(Server::default()); | ||
| let server_id = *server.id(); | ||
| let server_id = server.id().clone(); |
There was a problem hiding this comment.
These are all due to removing Copy from the BackendKeyData structs, right? I can't imagine clippy recommending this 😅
There was a problem hiding this comment.
This is not clippy, this is a refactor. I removed Copy from BackendKeyData, yup. It's now an enum, with the most likely variant resulting in a copy of 4 bytes, and the unlikely one an alloc of up to 256 bytes (variable secret key size in Postgres protocol 3.2 which will probably not be adopted for another decade).
| let id = if let Some(key_data) = key_data { | ||
| key_data | ||
| } else { | ||
| BackendKeyData::new() | ||
| }; |
There was a problem hiding this comment.
| let id = if let Some(key_data) = key_data { | |
| key_data | |
| } else { | |
| BackendKeyData::new() | |
| }; | |
| let id = key_data.unwrap_or_else(BackendKeyData::new) |
There was a problem hiding this comment.
That's actually what clippy complained about and told me not to do. It also made an incorrect automatic edit (unwrap_or_default()).
There was a problem hiding this comment.
Wait clippy complained about using unwrap_or_else? I would 100% expect it to complain about unwrap_or but not what I suggested. You could also do unwrap_or_else(|| BackendKeyData::new()) if passing the function pointer seems too clever but I would be shocked if clippy actually told you to write this instead of my suggestion
sgrif
left a comment
sgrif
left a comment
There was a problem hiding this comment.
I'd prefer that the semantic changes around BackendKeyData have been a separate PR, since doing that and clippy at the same time.
Given that we're keeping the same semantics as the code had before, this seems fine with one minor nit. However, I do think it'd be worth actually evaluating each of those clones to see if they really need to be passing owned data or not.
|
Found 1 test failure on Blacksmith runners: Failure
|
3 participants
BackendKeyDatasecret into an enum and removeCopy