-
Notifications
You must be signed in to change notification settings - Fork 567
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
Add #[data(eq)] shorthand attribute for Data derive macro #1884
Conversation
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 was definitely interested when this was initially proposed, but looking at it now I'm having second thoughts. Do you think this is a clear win?
docs/book_examples/src/data_md.rs
Outdated
@@ -21,7 +21,7 @@ struct TodoItem { | |||
due_date: Option<Arc<DateTime>>, | |||
// You can specify a custom comparison fn | |||
// (anything with the signature (&T, &T) -> bool). | |||
#[data(same_fn = "PartialEq::eq")] | |||
#[data(eq)] |
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.
Looking at this now, I'm not totally sure it's an improvement, or at least I'm not totally sure about this name. I think reading the code, it is very obvious what the old version does (especially since this sort of annotation is used in things like serde) but the second version looks very cryptic.
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 was also confused by naming, I like #[data(partial_eq)
more. But decided to go with the original naming.
I think this is a common case, We should do something about it. I am not sure if this is the best thing.
#[serde(default)]
and #[serde(default = "..")]
is similar to this.
and yes this comment needs to be updated
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.
Okay, I find that serde example compelling.
#[data(partial_eq)]
or #[data(partialeq)]
? #[data(eq)]
? #[data(equality)]
? #[data(equals)]
? #[data(==)]
? No, that's horrible.
so... #[data(partial_eq)]
or #[data(eq)]
, I guess.
An advantage of eq
is that once you know it exists it's harder to get wrong. So maybe it's better, after all? But I'm okay either way.
I agree that the docs should show examples of all attributes.
869acac
to
6a87cb6
Compare
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.
Works for me!
implement #1718