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.

Sign up for GitHub

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

Jump to bottom

Improve docs for crate features #1455

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Improve docs for crate features #1455

merged 1 commit into from
Feb 26, 2024

Conversation

edmorley
Copy link
Contributor

@edmorley edmorley commented Feb 23, 2024 •
edited
Loading

The chrono crate makes use of several Cargo features:

chrono/Cargo.toml

Lines 19 to 46 in 02c68d6

[features]
# Don't forget to adjust `ALL_NON_EXCLUSIVE_FEATURES` in CI scripts when adding a feature or an optional dependency.
default = ["clock", "std", "oldtime", "wasmbind"]
alloc = []
libc = []
winapi = ["windows-targets"]
std = ["alloc"]
clock = ["winapi", "iana-time-zone", "android-tzdata", "now"]
now = ["std"]
oldtime = []
wasmbind = ["wasm-bindgen", "js-sys"]
unstable-locales = ["pure-rust-locales"]
# Note that rkyv-16, rkyv-32, and rkyv-64 are mutually exclusive.
rkyv = ["dep:rkyv", "rkyv/size_32"]
rkyv-16 = ["dep:rkyv", "rkyv?/size_16"]
rkyv-32 = ["dep:rkyv", "rkyv?/size_32"]
rkyv-64 = ["dep:rkyv", "rkyv?/size_64"]
rkyv-validation = ["rkyv?/validation"]
# Features for internal use only:
__internal_bench = []
[dependencies]
num-traits = { version = "0.2", default-features = false }
rustc-serialize = { version = "0.3.20", optional = true }
serde = { version = "1.0.99", default-features = false, optional = true }
pure-rust-locales = { version = "0.8", optional = true }
rkyv = { version = "0.7.43", optional = true, default-features = false }
arbitrary = { version = "1.0.0", features = ["derive"], optional = true }

These features are documented in both README.md and the rustdocs in `lib.rs, however, the lists in each place were missing some features and also inconsistent in their descriptions.

I've also removed the unused [wasm-bindgen] link definition.

Fixes #1434.

@codecov Codecov
Copy link

codecov bot commented Feb 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.11%. Comparing base (02c68d6) to head (ed6fb25).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1455    /-   ##
=======================================
  Coverage   92.11%   92.11%           
=======================================
  Files          40       40           
  Lines       18026    18026           
=======================================
  Hits        16604    16604           
  Misses       1422     1422

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

@edmorley edmorley marked this pull request as draft February 23, 2024 11:25
@edmorley edmorley force-pushed the improve-feature-docs branch from 41f8df0 to 88eaa06 Compare February 23, 2024 11:37
@edmorley edmorley marked this pull request as ready for review February 23, 2024 11:38
@edmorley edmorley mentioned this pull request Feb 23, 2024
Closed
pitdicker
pitdicker reviewed Feb 23, 2024
View reviewed changes
Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Thank you @edmorley for straightening this out. One nit and one suggestion.

src/lib.rs Outdated
//! - `rkyv-32`: Enable serialization/deserialization via [rkyv], using 32-bit integers for integral `*size` types.
//! - `rkyv-64`: Enable serialization/deserialization via [rkyv], using 64-bit integers for integral `*size` types.
//! - `rkyv-validation`: Enable rkyv validation support using `bytecheck`.
//! - `rustc-serialize`: Enable serialization/deserialization via rustc-serialize (deprecated).
//! - `arbitrary`: construct arbitrary instances of a type with the Arbitrary crate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that everything starts with a capital, update this line also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot - I had skim read to look for more capitalisation issues, but my eyes clearly failed me 😆

src/lib.rs Outdated
//! - [`serde`][]: Enable serialization/deserialization via serde.
//! - `rkyv`: Enable serialization/deserialization via rkyv.
//! - `serde`: Enable serialization/deserialization via [serde].
//! - `rkyv`: Enable serialization/deserialization via [rkyv]. This is equivalent to `rkyv-32`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably discourage the use of this feature in the documentation, we only have it for compatibility. The replacement features where introduced in #1368.
No preference from my side on how to word it. Maybe just "rkyv: Deprecated, use the kkyv-* features"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

@edmorley edmorley force-pushed the improve-feature-docs branch from 88eaa06 to 1ca1df1 Compare February 23, 2024 12:55
@edmorley
Copy link
Contributor Author

I've force pushed rather than adding a "Fix review comments" commit since it looks like this repo doesn't normally use squash merges. To make the re-review easier, the diff between this version and the last can be seen here:
https://github.com/chronotope/chrono/compare/88eaa06ee4b8b5551efaceceaf62d7a5227eb0fd..1ca1df1654755f01402c5986802169802a1c7f3c

pitdicker
pitdicker approved these changes Feb 23, 2024
View reviewed changes
Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Thank you!
And thanks for looking at how we like the commits 😄.

@djc will probably also want to have a look.

djc
djc approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@djc djc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

Consistency is good, but on balance I would probably prefer to avoid capitalizing after :.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@edmorley
Improve docs for crate features
ed6fb25 
The `chrono` crate makes use of several Cargo features:
https://github.com/chronotope/chrono/blob/02c68d69a1ff8e6461384a770b87737b5096eae3/Cargo.toml#L19-L46

These features are documented in both `README.md` and the rustdocs
in `lib.rs, however, the lists in each place were missing some features
and also inconsistent in their descriptions.

I've also removed the unused `[wasm-bindgen]` link definition.

Fixes chronotope#1434.
@edmorley edmorley force-pushed the improve-feature-docs branch from 1ca1df1 to ed6fb25 Compare February 23, 2024 23:15
@edmorley
Copy link
Contributor Author

Consistency is good, but on balance I would probably prefer to avoid capitalizing after :.

So I tried switching to using lowercase after the colons, however, that didn't work so well for the items in the list that have multiple sentences after the colon, since then the first sentence is not capitalised, but later ones are.

Searching around, it seems that it's acceptable to capitalise after a colon when using a complete sentence:
https://www.grammarly.com/blog/capitalization-after-colons/

As such, I've added a couple of missing trailing periods, making all entries be complete sentences, so that they are suitable for capitalisation.

@edmorley
Copy link
Contributor Author

I've force pushed with the latest changes; diff here:
https://github.com/chronotope/chrono/compare/1ca1df1654755f01402c5986802169802a1c7f3c..ed6fb2592f66f497169ed9bb322d2c5e6e51643d

@pitdicker pitdicker merged commit 81d58e0 into chronotope:main Feb 26, 2024
35 checks passed
@pitdicker
Copy link
Collaborator

Thank you @edmorley.

@edmorley edmorley deleted the improve-feature-docs branch February 26, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitdicker pitdicker pitdicker approved these changes

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent crate features documentation
3 participants
@edmorley @pitdicker @djc