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

feat: Publicly expose DateTime #120

Merged
merged 1 commit into from
Aug 23, 2021
Merged

feat: Publicly expose DateTime #120

merged 1 commit into from
Aug 23, 2021

Conversation

epage
Copy link
Member

@epage epage commented Aug 23, 2021

This let's us test more cases we were already handling correctly.

Downside is that we are exposing chrono in the API.

I think we should flatten DateTime into Value but I want to defer
that conversation, rather than have it block all of this moving forward.

Fixes #113

This let's us test more cases we were already handling correctly.

Downside is that we are exposing `chrono` in the API.

I think we should flatten `DateTime` into `Value` but I want to defer
that conversation, rather than have it block all of this moving forward.

Fixes toml-rs#113
@ordian
Copy link
Member

ordian commented Aug 23, 2021

Downside is that we are exposing chrono in the API.

I think this was the reason why it wasn't exposed.
If the only reason to expose it is for tests, then maybe we could use cfg for that?

@epage
Copy link
Member Author

epage commented Aug 23, 2021

I think this was the reason why it wasn't exposed.
If the only reason to expose it is for tests, then maybe we could use cfg for that?

There are several API issues we'll have to resolve before the next release (I named another in the PR description). Can we instead open a general issue for what the API should look like and bundle all of those together, so these existing branches can continue moving forward?

@ordian
Copy link
Member

ordian commented Aug 23, 2021

Sure, sounds good.

@ordian ordian merged commit 65f01b7 into toml-rs:master Aug 23, 2021
@epage epage deleted the date-pub branch August 23, 2021 18:09
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

Successfully merging this pull request may close these issues.

Users can't determine what kind of DateTime is included
2 participants