Add PartialEq/Eq, Add TimeUnitSpecEq #78
Conversation
src/schedule.rs
Outdated
|
|
||
| impl PartialEq for TimeUnitSpecEq { | ||
| fn eq(&self, other: &TimeUnitSpecEq) -> bool{ | ||
| self.0.fields == other.0.fields |
There was a problem hiding this comment.
Rather than vending a new type that achieves this, how would you feel about having something like:
impl Schedule {
// ...
pub fn ordinals_eq(other: &Schedule) -> bool {
self.fields == other.fields
}
// ...
}and allowing users to define their own wrapper type that implements PartialEq in terms of ordinals_eq? That alleviates the pressure to provide a type with a perfect name.
There was a problem hiding this comment.
Nitpick: Maybe we should call it timeunitspec_eq? Because Ordinals is not exposed?
There was a problem hiding this comment.
Nitpick: Maybe we should call it timeunitspec_eq? Because Ordinals is not exposed?
Oh, good point! I wish I liked the type name TimeUnitSpec more, but we'll still have time to revise that before 1.0 if I can think of something better. Let's go with timeunitspec_eq.
src/time_unit/days_of_month.rs
Outdated
| @@ -5,7 +5,7 @@ use once_cell::sync::Lazy; | |||
|
|
|||
| static ALL: Lazy<OrdinalSet> = Lazy::new(|| { DaysOfMonth::supported_ordinals() }); | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| #[derive(Clone, Debug, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Deriving PartialEq means that some equivalent representations of "all" may not be found to be equal. (e.g. * and ? would be considered equal, but * and 1-7 for DayOfWeek would not.)
We could address this by either:
- Manually implementing
PartialEqand checking for theis_all()case OR - Adding code to the parsing logic that checks to see if we're about to call
from_ordinal_set()using an ordinal set that's effectively "all" and usingall()instead, guaranteeing that the internal representation is the same.
I think I'd prefer the former as it consolidates the definition of PartialEq into a single function while the latter relies on knowing about an invariant in the parser. It seems like we could do a single blanket impl of PartialEq?
impl <T> PartialEq for T where T: TimeUnitField { /*....*/ }With that in place, ScheduleFields could use derive(PartialEq, Eq).
There was a problem hiding this comment.
You're right, good catch! I can do with both ideas, so I'll work on the former
There was a problem hiding this comment.
Sadly it seems that
impl <T> PartialEq for T where T: TimeUnitField { /*....*/ }runs into some conflicts. I can go forward and implement it individually though
see: https://stackoverflow.com/questions/62904267/conflicting-implementation-error-with-single-implementation
There was a problem hiding this comment.
Oof, I've stubbed my toe on that more than once and still forgot about it.
|
Great, I think that'll be my last PR this cycle. Maybe I can work on #77 , but otherwise I'm done. This was a great way of getting to know rust |
|
No problem, it was fun! I can open a draft pull request with the code I have up until now if you want 🙂🤔 |
Fixes #61
Not sure about the name TimeIUnitSpecEq, but that can be changed.