-
Notifications
You must be signed in to change notification settings - Fork 93
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
[TRIVIAL] Format hex encoding with derive_more #3273
Conversation
WalkthroughThis pull request introduces two changes in the database crate. In the Cargo.toml file, a new dependency, derive_more, is added with workspace configuration. In the byte_array.rs file, the ByteArray struct is updated to automatically derive the Debug trait using derive_more, replacing a custom implementation. The changes simplify import formatting and maintain existing functionality. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🔇 Additional comments (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ce427e6
to
47a3fb0
Compare
47a3fb0
to
29a3404
Compare
} | ||
#[derive(Clone, Copy, Eq, PartialEq, Hash, sqlx::FromRow, derive_more::Debug)] | ||
pub struct ByteArray<const N: usize>( | ||
#[debug("0x{}", hex::encode::<&[u8]>(self.0.as_ref()))] pub [u8; N], |
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.
Originally I wanted to make this PR to change it in all the occurrences, but it cannot be changed for the occurrences which have a generic type (e.g. pub Type<T>(T)
).
The reason is that hex::encode()
requires the implementation of AsRef<[u8]>
. So, theoretically we could do pub Type<T: AsRef<[u8]>(T)
, but it won't work. The reason is that the macro expands before the compiler acknowledges the trait limitation. This is a limitation in the derive_more
crate which does not acknowledge struct-level bounds.
So, in the end this PR only changes one occurrence. I am up to close it if it does not add any benefit.
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.
I've accepted the suggestion in one of my previous PRs, but I am not sure about this change as it kills readability a bit (at least to me).
I agree. The |
@MartinquaXD @sunce86 I understand your point, totally valid. What do you guys think of having a function to do this conversion, and the derive more points to that function (the same as |
Not sure this is addressing an issue we have at the moment. I'd prefer to focus our time on actually fixing alerts for now. When we are back to a sustainable state we can discuss how to make logging nicer. |
Description
Use the crate
derive_more
in order to avoid repeatedDebug
implementation for the hex encoding.Summary by CodeRabbit