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

Optionally remove time crate dependency #236

Merged
merged 5 commits into from
Apr 3, 2018

Conversation

jethrogb
Copy link

@jethrogb jethrogb commented Apr 2, 2018

Continuation of #137. I had to keep the old branch around for... reasons.

Original PR:


I would like to use the chrono crate to process time information on systems that don't have a clock. The time crate, which is used by chrono to obtain and represent the local time, can't be made to work on such a system. This PR adds a default-enabled "local" feature to the crate and moves all code that is dependent on the time crate behind a cfg-gate. Current users of chrono need to do nothing. Users of chrono that would like to use it without the time crate can set default-features = false in their Cargo.toml.

Open questions:

  • Test coverage I have fixed all unit tests such that they will pass with or without the local feature. There are however 31 doc tests that use some code that has been cfged-out. I'm not sure how to selectively disable them.
  • UTC UTC::now and UTC::today also depend on the time crate. I have put them behind the same cfg-gate, but that might be a misnomer. We could leave this as is, or I could add another feature with a different name or rename the current feature.

@jethrogb
Copy link
Author

jethrogb commented Apr 3, 2018

Ah, I see we're not running with cargo test --no-default-features at all. I agree that that is unfortunate. There's no way to just not run the doctests either, afaict. I would strongly prefer to test with --no-default-features, I'll do some more research.

@quodlibetor What do you mean? I changed the way CI is run in 5e68c60#diff-b4553af39acae935d505c76e139d4aa3

@quodlibetor
Copy link
Contributor

Oh I'm just confused. Yeah what you've currently got seems like as ideal as we're going to get.

@quodlibetor quodlibetor merged commit 73ba5d7 into chronotope:master Apr 3, 2018
@quodlibetor
Copy link
Contributor

I'm really busy this week, but will release chrono 0.4.2 with this feature by the weekend.

Thanks so much for your patience!

@quodlibetor
Copy link
Contributor

This is now in crates.io as chrono 0.4.2.

Thanks again for your patience!

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.

2 participants