Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Date & Time #19

Closed
zeenix opened this issue Nov 26, 2024 · 10 comments · Fixed by #35
Closed

Date & Time #19

zeenix opened this issue Nov 26, 2024 · 10 comments · Fixed by #35

Comments

@zeenix
Copy link
Owner

zeenix commented Nov 26, 2024

This should have been supported from the day 1. I think we need to take the user-facing API from toml_datetime (so we don't need to depend on another external crate) and perhaps also take the parsing code from toml_edit (with some adaptation of course).

I think this will also fix a bunch of failing cases in the from the test harness (#11).

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

cc @CosmicHorrorDev. BTW, if you haven't already, I'd suggest subscribing to all activity on this repo since you said you intend to help develop and maintain it. :)

@CosmicHorrorDev
Copy link
Contributor

I would personally just depend on toml_datetime at that point. Virtually the entire crate is it's public API

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

I would personally just depend on toml_datetime at that point. Virtually the entire crate is it's public API

Good point.

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

I would personally just depend on toml_datetime at that point. Virtually the entire crate is it's public API

Good point.

It's not a no_std lib though so we'll need to make it one, to use it.

@CosmicHorrorDev
Copy link
Contributor

Ah I didn't realize 😅

It looks like it only needs alloc, so I can handle working with upstream on that

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

It looks like it only needs alloc, so I can handle working with upstream on that

Yeah, I asked the maintainer in a PM if he'd be interested in a PR for that. Unlikely that he's not but you never know. :)

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

It looks like it only needs alloc, so I can handle working with upstream on that

Yeah, I asked the maintainer in a PM if he'd be interested in a PR for that. Unlikely that he's not but you never know. :)

He says, he's good with that. 👍 So please go ahead.

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

He says, he's good with that. 👍 So please go ahead.

One issue though: Error trait only became stable in core in Rust 1.81, which is way too recent and I doubt Ed would be happy to bump from 1.65 to 1.81 just for this. In tomling, I added an std feature only for this and Error trait is implemented if std is enabled. However, that would be a breaking change and seems Ed (understandably) isn't too enthusiastic about that. Unless there is another solution to this issue that doesn't involve breaking changes or too big an MSRV bump, I'm afraid we'll be only left with my first suggestion. :(

Btw, Ed said to add #![warn(clippy::std_instead_of_core)] to the crate if any std API will still be used. I already did that for tomling.

@CosmicHorrorDev
Copy link
Contributor

Ah yeah that's a good point 😿

Whelp then I think we carry on with copying out the parts we need

@zeenix
Copy link
Owner Author

zeenix commented Nov 26, 2024

Whelp then I think we carry on with copying out the parts we need

Yeah. 🤷 Still if we try and keep modifications to minimum, we can ditch our copy once toml_edit can bump its MSRV to 1.81.

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

Successfully merging a pull request may close this issue.

2 participants