-
Notifications
You must be signed in to change notification settings - Fork 301
Support infinite intervals #759
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
Support infinite intervals #759
Conversation
josevalim
left a comment
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 am a bit torn if we should do it like this or encode them in Postrex.Interval. Or perhaps make them opt-in?
|
Ah, yes, we do this for timestamps: So we shoud have |
|
@josevalim done! |
lib/postgrex/extensions/interval.ex
Outdated
| other -> | ||
| raise ArgumentError, | ||
| "#{inspect(other)} is not valid for `:interval_decode_type`. Please use either `Postgrex.Interval` or `Duration`" | ||
| unless type in [Postgrex.Interval, Duration] do |
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.
| unless type in [Postgrex.Interval, Duration] do | |
| if type not in [Postgrex.Interval, Duration] do |
There were a couple of people asking for this so I took a stab.
There is one thing I think can be debated. For timestamps we have an option to opt into infinity values. I'm not 100% sure the reason so I'm not sure if it's the right thing to do for intervals.
One reason I can see for adding an option is if people were using these values in the past as a proxy for +/- infinity and now they will be returned as atoms instead of structs. But on the flipside most people will probably want the atoms if they are using this behaviour and if we make it opt in it will be more annoying. So I was thinking it might make sense to leave it like this and then add an opt out option if people are having issues.