Implement system macro make_timestamp #988
Open
+520
−26
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #942 (and a little of #970)
Description of changes:
This PR implements the
make_timestamp
system macro. It's a bigger PR than I really wanted, but I also wanted something to handle the timestamp builder a little better.There are a few changes in here that are not directly related to
make_timestamp
, but do get used in the implementation:Sub
forDecimal
. I originally added this thinking some arithmetic operations on Decimals would make handling the fractional second Decimal nicer. Ultimately I didn't use it.fract
andtrunc
to Decimal to handle extracting the whole and fractional values from the decimal. This mirrors the floating point types in Rust.SystemMacroArguments
type to be a little more generic. It takes a generic argument that isAsRef<str>
, which lets you provide a type that can be used to return a&str
for the parameter name. In themake_timestamp
implementation this is used to provide an enum that can be used to provide both the argument's position, as well as the name. Right now the only 2 implementations that are using it aremake_decimal
andmake_timestamp
, so it seems a bit overkill, but I'll circle back around and revisit the other system macros where it could be used.The
TimestampBuilderWrapper
type that I'm using to handle the changingTimestampBuilder
types adds a lot of bulk to the diff and I'm not too thrilled about that.Conformance Test
Conformance tests for
make_timestamp
are passing:CLI
I have a local edit of the ion-cli to work with ion-rust HEAD. So I've been able to test the newer system macros that weren't part of rc6:
This has been super helpful.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.