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

Override equal? for time based ranges #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camilleryr
Copy link

Hello!

First off - thanks for this project, it has been invaluable in our application and we really appreciate the work you have put into this. We recently ran into a small issue caused by the default (provided by Ecto.Type) implementation of TsRange.equal?. The default implementation simple checks for strict equality (==) which is not recommended for for Date/DateTime structs (per the elixir docs Remember, comparisons in Elixir using == and friends are structural and based on the DateTime struct fields. For proper comparison between datetimes, use the compare/2 function.)

This PR adds an override for the equal? function for TsRange, TstzRange and DateRange. Let me know if you would like to see any changes to this, or feel free to reject it entirely if you do not find this to be a positive addition to the project

@maennchen
Copy link

@camilleryr I've just opened a PR to handle empty / unbounded ranges correctly. This PR can not deal with that yet. Would you be willing to handle :empty and :unbound as well or should I open a new PR for that?

@vforgione
Copy link
Owner

@camilleryr I think this is a good idea. I've asked @maennchen to add some additional tests to his PR (#17). Similarly, could you add tests to yours as well?

I'd like to get @maennchen's PR merged in first and then this, so it may be best to hold off until that is in so your tests accommodate those changes. The addition of :empty could simplify some of the logic in this.

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.

3 participants