-
Notifications
You must be signed in to change notification settings - Fork 28
Add Range type, encoder and decoder #70
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
base: main
Are you sure you want to change the base?
Conversation
src/pog.gleam
Outdated
| coerce_value(#(time.hours, time.minutes, seconds)) | ||
| } | ||
|
|
||
| pub fn range(converter: fn(a) -> Value, range: Range(a)) { |
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 aligned the argument order with pog.array, but please check if that’s correct.
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.
Add the missing type annotation please 🙏
lpil
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.
Thank you! Could you update the changelog also please
src/pog.gleam
Outdated
| coerce_value(#(time.hours, time.minutes, seconds)) | ||
| } | ||
|
|
||
| pub fn range(converter: fn(a) -> Value, range: Range(a)) { |
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.
Add the missing type annotation please 🙏
| pub type Inclusivity { | ||
| Inclusive | ||
| Exclusive | ||
| } |
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.
Document the new data types please 🙏
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.
Done. Please double check if it's correct and clear.
src/pog.gleam
Outdated
| use decoded_atom <- decode.then(atom.decoder()) | ||
| case decoded_atom == atom.create("empty") { | ||
| True -> decode.success(Empty) | ||
| False -> decode.failure(Empty, "empty") |
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.
What's empty? Shouldn't this be Range?
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.
As far as I understand, this is a sub decoder designed to decode only a single empty Erlang atom. Its error is always suppressed by the usage of decode.one_of.
I've changed the expected error message to be clearer:
False -> decode.failure(Empty, "`empty` atom")I've also changed the main/parent decoder to collapse errors into something more readable:
decode.one_of(range_decoder, [empty_decoder])
|> decode.collapse_errors("`#(#(t, t), #(bool, bool))` tuple or `empty` atom")Is it better now or could you please explain what I'm missing here?
| ) | ||
| |> disconnect | ||
| } | ||
|
|
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.
Could you add tests for each of the possible ranges that could be decoded please 🙏 Using postgresql literals sounds good.
src/pog.gleam
Outdated
| use decoded_atom <- decode.then(atom.decoder()) | ||
| case decoded_atom == atom.create("unbound") { | ||
| True -> decode.success(Unbound) | ||
| False -> decode.failure(Unbound, "unbound") |
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 think this should be Bound
Add tests
| // this test doesn't work! | ||
| |> assert_roundtrip( | ||
| pog.Range(pog.Bound(1.23, pog.Exclusive), pog.Unbound), | ||
| "numrange", | ||
| encoder, | ||
| decoder, | ||
| ) | ||
| // this test doesn't work! | ||
| |> assert_roundtrip( | ||
| pog.Range(pog.Bound(-3.14, pog.Exclusive), pog.Bound(3.14, pog.Inclusive)), | ||
| "numrange", | ||
| encoder, | ||
| decoder, | ||
| ) |
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.
These tests are failing with the following errors:
An unexpected error occurred:
Badmatch(<<0, 0, 0, 12, 0, 2, 0, 0, 0, 0, 0, 2, 0, 1, 8, 252>>)
An unexpected error occurred:
Badmatch("\u{0000}\u{0000}\u{0000}\f\u{0000}\u{0002}\u{0000}\u{0000}@\u{0000}\u{0000}\u{0002}\u{0000}\u{0003}\u{0005}x\u{0000}\u{0000}\u{0000}\f\u{0000}\u{0002}\u{0000}\u{0000}\u{0000}\u{0000}\u{0000}\u{0002}\u{0000}\u{0003}\u{0005}x")
This appears to be a bug, but I'm not sure where the root cause is.
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.
The issue is on the pg_types side. I've created tsloughter/pg_types#21 to track its progress.
This PR introduces a generic
Range(t)type to support Postgres range types.Please let me know if there's anything I can improve.