feat: support int4range type #190
Conversation
This is an exploratory effort to support will#164 It is not ready for merging, but I wanted to to open it for discussion and review. Signed-off-by: Mike Fiedler <[email protected]>
miketheman
force-pushed
the
miketheman/range-types
branch
from
e883b64 to
46eacd0
Compare
|
|
||
| def type | ||
| # TODO: do I actually need a new PG-specific type, or can we use real Crystal Ranges? | ||
| Range |
There was a problem hiding this comment.
Should this be a specifically named Int4Range, or should the Range decoder emit a Crystal Range? Is the expectation that the end user desire to be super-specific about whether they are passing an Integer, BigInt, or Numeric type?
There was a problem hiding this comment.
It's been a while since I've looked into ranges in crystal, but I think maybe postgres ranges have more "features" than crystal ranges.
In some cases, it is probably easy to normalize a postgres range into a crystal one, like how in the tests you have here the inclusive/exclusive adding 1. However in other cases it might not work, like for the float ranges and date ranges allowing positive and negative infinity. We may have to make our own range class for those. Or enhance the crystal stdlib ranges.
There was a problem hiding this comment.
Range can take nil as start and/or end value, so maybe this could be used to represent infinity.
| @@ -0,0 +1,10 @@ | |||
| require "../../spec_helper" | |||
There was a problem hiding this comment.
I created a new spec file to help organize the range specs outside of the regular specs - it looked like that's what happened with array_decoder_spec.cr
I guess if this turns into multiple structs for the different ranges, then maybe the decoder ought to be extracted as well?
There was a problem hiding this comment.
Yeah I don’t have any fully thought out reason, but it kinda makes sense since there will be a lot of related decoders for the ranges.
| @@ -0,0 +1,10 @@ | |||
| require "../../spec_helper" | |||
There was a problem hiding this comment.
Yeah I don’t have any fully thought out reason, but it kinda makes sense since there will be a lot of related decoders for the ranges.
|
|
||
| def type | ||
| # TODO: do I actually need a new PG-specific type, or can we use real Crystal Ranges? | ||
| Range |
There was a problem hiding this comment.
It's been a while since I've looked into ranges in crystal, but I think maybe postgres ranges have more "features" than crystal ranges.
In some cases, it is probably easy to normalize a postgres range into a crystal one, like how in the tests you have here the inclusive/exclusive adding 1. However in other cases it might not work, like for the float ranges and date ranges allowing positive and negative infinity. We may have to make our own range class for those. Or enhance the crystal stdlib ranges.
Signed-off-by: Mike Fiedler <[email protected]>
This is an exploratory effort to support #164
It is not ready for merging, but I wanted to to open it for discussion
and review.