Closed rmsyn closed 1 year ago
Thank you for your PR! I will review this PR within a week.
BTW, I fixed clippy warnings in https://github.com/sile/libflate/pull/69. So, please merge the master branch to fix CI lint failures.
BTW, I fixed clippy warnings in https://github.com/sile/libflate/pull/69
nice
This PR basically seems good. However, this change always adds extra dependencies core2
and hashbrown
(those haven't reached the v1 release) even when users, who want to use the "std" feature, don't actually need that.
I want to be cautious to add mandatory dependencies. So how about adding "no_std" feature and making core2
and hashbrown
optional dependencies instead?
Thanks for the review.
So how about adding "no_std" feature and making core2 and hashbrown optional dependencies instead?
That makes sense to me. Wasn't sure if following the default std
feature pattern from other crates was better, or the optional dependency + no_std
feature.
The two fixup commits implement the changes you requested, and remove the default std
feature. Leaving in the std
feature resulted in the slightly awkward cargo build/test --no-default-features --features no_std
for no_std
builds (default-features = false, features = ["no_std"]
for library usage). So, I removed the std
feature.
If you like the changes, I'll squash them into the original commit.
Thank you for the changes! LGTM 👍
I'll squash them into the original commit.
I will merge this PR once you do that.
That makes sense to me. Wasn't sure if following the default std feature pattern from other crates was better, or the optional dependency + no_std feature.
That is definitely the better pattern to follow - cargo features are intended to be additive, since if multiple libraries depend on different features in a library, both features are enabled . A no_std
feature is the opposite of this.
Please consider redoing this PR the way most of the community follows and Cargo intends.
This PR also doesn't fix #![no_std]
builds, since the crc32fast
and adler32
dependencies define default-enabled std
features that this repo doesn't default-features = false
. There's not really an easy way to make a no_std
feature work with these dependencies; the best way to fix this is to switch to an optional std
feature.
Leaving in the std feature resulted in the slightly awkward cargo build/test --no-default-features --features no_std for no_std builds (default-features = false, features = ["no_std"] for library usage). So, I removed the std feature.
The way to alleviate this is to make std
a non-default feature and require users that want std
features to include the optional std
feature. std
code works with #![no_std]
code, but not the other way around.
@kupiakos Thank you for your comment.
cargo features are intended to be additive, since if multiple libraries depend on different features in a library
Make sense.
I'm willing to remove the no_std
feature and introduce a std
feature instead (and release the next major version as it would be a breaking change).
Are you interested in creating a PR for this change?
I'm willing to remove the
no_std
feature and introduce astd
feature instead (and release the next major version as it would be a breaking change).
I can take a look sometime this next week. Which option of these would you prefer @sile ?
std
a non-default feature and require users that want std
features to include the optional std
feature.std
a default feature and require #![no_std]
users to default-features = false
.Great!
Which option of these would you prefer?
I don't have a strong opinion on which one is the better choice at this point. Please choose the one you think is most appropriate and let's discuss it in the PR if need.
That is definitely the better pattern to follow - cargo features are intended to be additive, since if multiple libraries depend on different features in a library, both features are enabled . A no_std feature is the opposite of this.
Please consider redoing this PR the way most of the community follows and Cargo intends.
Yeah, this is how the original PR was. The conversion isn't too involved (basically a few global sed
invocations).
Let me know if you would like me to make the PR, or if you want to. I'm good with either, and appreciate your review.
This PR also doesn't fix #![no_std] builds, since the crc32fast and adler32 dependencies define default-enabled std features that this repo doesn't default-features = false. There's not really an easy way to make a no_std feature work with these dependencies; the best way to fix this is to switch to an optional std feature.
The library still technically builds, but yes, I missed these dependencies bringing in std
by default. In the follow-up to this PR, I agree with you that the indirect dependency on std
in no_std
builds should be eliminated.
The way to alleviate this is to make std a non-default feature and require users that want std features to include the optional std feature. std code works with #![no_std] code, but not the other way around.
Right, I missed those dependencies you mentioned. Good catch :+1:
@rmsyn, @sile, please take a look at #74
Introduce a new, default
std
feature to the library, and conditionally use structures and traits fromno_std
compatible libraries.Existing
std
users can continue using the library with no changes.no_std
users can import the library usingdefault_features = false
.Changes were testing by running the test suite using the
std
andno_std
configurations: