Skip to content
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

Added partialeq and eq derives #62

Closed
wants to merge 1 commit into from
Closed

Conversation

celaus
Copy link

@celaus celaus commented Nov 29, 2020

Fixes #61

@zslayton
Copy link
Owner

zslayton commented Dec 4, 2020

Hi, thanks for the PR.

Using a derive implementation of Eq/PartialEq means that two Schedules will only be equal if:

  1. They represent the same set of datetimes
    AND
  2. The source strings that were parsed to create the two Schedules are identical.

This might be a narrower definition of equality than we want, since you could have a schedule with a day field of SUN,MON,TUES,WED,THU,FRI,SAT and one with * and they'd represent the same conceptual schedule. Then again, I could see people reasonably arguing that the source string is part of Schedule equality.

Any thoughts on this?

@celaus
Copy link
Author

celaus commented Dec 6, 2020

Hmm true, that might be a too narrow definition.

I'd say if two instances represent the same datetimes and one has a source string and the other doesn't, they should still be equal. However I don't know cron syntax well enough to know if there is an example for the other way around as well, i.e. when the source strings are different but result in the same datetime representations...

Otherwise I'd say we compare source strings when available and datetime (only) for the rest?

I'm happy to implement whatever definition we agree on 😄

@zslayton
Copy link
Owner

Otherwise I'd say we compare source strings when available and datetime (only) for the rest?

The source string will always be available. (The source string is an Option in the struct as a bit of a workaround).

I'm inclined to say let's implement it using source string equality for now. Determining the equality of the cron schedule the string represents is much trickier, especially if the parser adds support for other syntaxes in the future.

Out of interest, what's your main use case for PartialEq and Eq?

@celaus
Copy link
Author

celaus commented Jan 27, 2021

I wanted to keep actions that run on the same schedule as a HashMap<Schedule, Vec<ExecuteOnSchedule> 😄
(I haven't worked on that particular project for a while)

@zslayton
Copy link
Owner

PartialEq and Eq implementations were added in PR #78.

@zslayton zslayton closed this Feb 21, 2021
@celaus celaus deleted the eq branch February 23, 2021 16:00
@celaus
Copy link
Author

celaus commented Feb 23, 2021

Thank you everyone for pitching in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PartialEq and Eq for the Schedule type
2 participants