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
Fix #473: add explicit serde feature that is independent from dependency #524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me, including the comment I posted here: #473 (comment)
## This feature does NOT provide XML serializer or deserializer. You should use | ||
## the `serialize` feature for that instead. | ||
# Cannot name "serde" to avoid clash with dependency. | ||
# "dep:" prefix only avalible from Rust 1.60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that with edition 2021 / MSRV 1.60, it is possible to simplify all of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in Rust 1.60 it is possible to write
[dependencies]
serde = { version = "1.0", optional = true }
[features]
# depend on the dependency "serde" instead of feature "serde"
# (which will enable "derive" feature of "serde" which is not required
# for the "serialize" feature)
serialize = ["dep:serde"]
# depend on "serde" dependency with enabled feature "derive"
serde = ["serde/derive"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't want to bump the MSRV right now then we should file an issue to simplify this in the future. I wouldn't be against bumping MSRV though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mingun ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create an issue if you wish. Anyway, I marked this place with Rust version that is required for more modern solution, so it can be grepped.
I think, this that having more modern solution is not critical, so it is better to avoid bumping MSRV just for it. Otherwise, probably some of our dependents will not happy with that.
This should fix #473. @loyd, @xkr47, can you check?
Checked by
(failed without
--path
with the fix)