Current implementation of Range has unnecessary structs/enums that make it confusing to use and maintain. Range was an enum of the possible types of ranges, and was used in places where there should only be one type of range. The Rust-y thing to do would be to rely on generics/monomorphization of RangeImpl. (This is also problem in the Kotlin implementation that was ported over when ion-schema-rust was in its early days.)
In addition, TypedRangeBoundaryValue and RangeBoundaryValue had a lot of redundancy, and RangeBoundaryType models inclusivity/exclusivity, but it's not truly orthogonal with the boundedness/unboundedness (which is modeled in RangeBoundaryValue and TypedRangeBoundaryValue).
Major changes in this PR include:
There's no longer an untyped range
Collapsed the exclusivity and the boundedness into a single enum (Limit)
Updated all range-based constraints to have the appropriate type of range (e.g. Utf8ByteLength(Range) => Utf8ByteLength(UsizeRange))
Change ValidValue to be an enum of Element, NumberRange, and TimestampRange – this is required because there's no longer a single range type that covers all ranges.
Change ValidValue::Element to hold a Value. Now it's impossible to construct a valid values constraint that has annotations on a ValidValue::Element. (May as well use the type system to our advantage.)
There's also PR tour notes in the PR diff that call out smaller changes.
Some things that I don't have a strong opinion about and/or want feedback:
Naming of Limit – it could also be Boundary and the variants could be Unbounded, Inclusive, and Exclusive.
Should the precision constraint use NonZeroU64 instead of u64? I wasn't sure if the benefit or the non-zero type would outweigh the loss of the convenience of using a primitive (u64) for this.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue #, if available:
N/A
Description of changes:
Current implementation of Range has unnecessary structs/enums that make it confusing to use and maintain.
Range
was an enum of the possible types of ranges, and was used in places where there should only be one type of range. The Rust-y thing to do would be to rely on generics/monomorphization ofRangeImpl
. (This is also problem in the Kotlin implementation that was ported over whenion-schema-rust
was in its early days.)In addition,
TypedRangeBoundaryValue
andRangeBoundaryValue
had a lot of redundancy, andRangeBoundaryType
models inclusivity/exclusivity, but it's not truly orthogonal with the boundedness/unboundedness (which is modeled inRangeBoundaryValue
andTypedRangeBoundaryValue
).Major changes in this PR include:
Limit
)Utf8ByteLength(Range)
=>Utf8ByteLength(UsizeRange)
)ValidValue
to be an enum ofElement
,NumberRange
, andTimestampRange
– this is required because there's no longer a single range type that covers all ranges.ValidValue::Element
to hold aValue
. Now it's impossible to construct a valid values constraint that has annotations on aValidValue::Element
. (May as well use the type system to our advantage.)There's also PR tour notes in the PR diff that call out smaller changes.
Some things that I don't have a strong opinion about and/or want feedback:
Limit
– it could also beBoundary
and the variants could beUnbounded
,Inclusive
, andExclusive
.precision
constraint useNonZeroU64
instead ofu64
? I wasn't sure if the benefit or the non-zero type would outweigh the loss of the convenience of using a primitive (u64
) for this.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.