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

0.5: Revise exposed crate features #1552

Merged
merged 10 commits into from
Apr 7, 2024

Conversation

pitdicker
Copy link
Collaborator

Remove the old crate features libc, old_time, rkyv and winapi.
Switch to the dep: syntax for optional dependencies that where exposed as features: pure-rust-locales, android-tzdata, iana-time-zone, js-sys, wasm-bindgen and windows-targets.

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.99%. Comparing base (58ae538) to head (87f8b43).
Report is 1 commits behind head on 0.5.x.

Additional details and impacted files
@@           Coverage Diff           @@
##            0.5.x    #1552   +/-   ##
=======================================
  Coverage   93.99%   93.99%           
=======================================
  Files          37       37           
  Lines       16536    16543    +7     
=======================================
+ Hits        15543    15550    +7     
  Misses        993      993           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker pitdicker force-pushed the revise_exposed_features branch 3 times, most recently from dc0d280 to 591a995 Compare March 31, 2024 12:52
@pitdicker
Copy link
Collaborator Author

Also updated the description in lib.rs and the readme, and removed the __unstable_bench feature.

The bench feature was only used for benchmarking YearFlags::from_year. All that method does is:

let year = year.rem_euclid(400);
YEAR_TO_FLAGS[year as usize]

A division and a table lookup doesn't seem all that interesting to benchmark, let alone have a feature for.

Cargo.toml Show resolved Hide resolved
@pitdicker pitdicker force-pushed the revise_exposed_features branch 2 times, most recently from 86d06a3 to ff2057e Compare April 2, 2024 12:45
Cargo.toml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Apr 3, 2024

Much better, I think that split was definitely worth it!

README.md Outdated Show resolved Hide resolved
@pitdicker pitdicker mentioned this pull request Apr 4, 2024
@pitdicker pitdicker force-pushed the revise_exposed_features branch 5 times, most recently from ff64945 to c30e360 Compare April 4, 2024 10:56
@pitdicker
Copy link
Collaborator Author

Sorry that this is taking so many back-and-fort between us. But I'm really starting to like the changes here as good cleanups.

@@ -27,7 +27,7 @@ clock = [
"dep:android-tzdata",
"now",
]
now = ["std"]
now = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we should revert this change, because now without std is pretty much meaningless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the commit I added now disables no_std, with the goal that we only depend on the standard library if the implementation for that platform requires it.

So the features describe the functionality: now enables Utc::now, std enables the Error trait and TryFrom<SystemTime>.

Is that sensible or do you really want to revert?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now should enable calling Utc::now() on most common platforms. If it doesn't, that feels broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems surprising that the cfg(feature = "std") guard doesn't match the behavior of whether we enable/disable no_std exactly (for the library).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a choice we can make. Separating the features allows things like using Utc::now in combination with wasmbind without depending on the standard library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's try it. Easier to add it later than to remove it.

@pitdicker pitdicker merged commit 332be72 into chronotope:0.5.x Apr 7, 2024
35 checks passed
@pitdicker pitdicker deleted the revise_exposed_features branch April 7, 2024 06:44
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.

None yet

2 participants