Skip to content
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

Change type of Timestamp::fractional_seconds to be Mantissa #411

Open
desaikd opened this issue Sep 1, 2022 · 2 comments
Open

Change type of Timestamp::fractional_seconds to be Mantissa #411

desaikd opened this issue Sep 1, 2022 · 2 comments

Comments

@desaikd
Copy link
Contributor

desaikd commented Sep 1, 2022

Current implementation of Timestamp has fractional_seconds type as Option<Mantissa>.

https://github.com/amzn/ion-rust/blob/944511591310f7e273074eaa252f6cd37b5728bb/src/types/timestamp.rs#L126-L131

The above could be simplified by making type of fractional_seconds as Mantissa when creating the Timestamp. We can put a default value for fractional_seconds which could either be Mantissa::Digits(0) or Mantissa::Decimal(Decimal::new(0,0)).

Reference PR: #410

@zslayton
Copy link
Contributor

zslayton commented Jul 5, 2023

This is an implementation detail; I don't believe it needs to block the 1.0 release. Thoughts, @popematt?

@popematt popematt removed this from the v1.0 Release milestone Jul 5, 2023
@popematt
Copy link
Contributor

popematt commented Jul 5, 2023

Now that I've looked a little closer, I think you're right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants