Skip to content

Comments

Fix: the serialize feature should also enable serde/derive#472

Closed
loyd wants to merge 1 commit intotafia:masterfrom
loyd:master
Closed

Fix: the serialize feature should also enable serde/derive#472
loyd wants to merge 1 commit intotafia:masterfrom
loyd:master

Conversation

@loyd
Copy link

@loyd loyd commented Sep 7, 2022

In master/last release serialize feature implicitly depends on serde/derive.

Just add resolve = "2" to Cargo.toml to see the following:

error[E0433]: failed to resolve: could not find `Deserialize` in `serde`
  --> src/name.rs:19:45
   |
19 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
   |                                             ^^^^^^^^^^^ could not find `Deserialize` in `serde`

error[E0433]: failed to resolve: could not find `Serialize` in `serde`
  --> src/name.rs:19:65
   |
19 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
   |                                                                 ^^^^^^^^^ could not find `Serialize` in `serde`

error[E0433]: failed to resolve: could not find `Deserialize` in `serde`
   --> src/name.rs:136:45
    |
136 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                             ^^^^^^^^^^^ could not find `Deserialize` in `serde`

error[E0433]: failed to resolve: could not find `Serialize` in `serde`
   --> src/name.rs:136:65
    |
136 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                                                 ^^^^^^^^^ could not find `Serialize` in `serde`

error[E0433]: failed to resolve: could not find `Deserialize` in `serde`
   --> src/name.rs:186:45
    |
186 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                             ^^^^^^^^^^^ could not find `Deserialize` in `serde`

error[E0433]: failed to resolve: could not find `Serialize` in `serde`
   --> src/name.rs:186:65
    |
186 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                                                 ^^^^^^^^^ could not find `Serialize` in `serde`

error[E0433]: failed to resolve: could not find `Deserialize` in `serde`
   --> src/name.rs:228:45
    |
228 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                             ^^^^^^^^^^^ could not find `Deserialize` in `serde`

error[E0433]: failed to resolve: could not find `Serialize` in `serde`
   --> src/name.rs:228:65
    |
228 | #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
    |                                                                 ^^^^^^^^^ could not find `Serialize` in `serde`

@loyd loyd changed the title Fix: the serde feature should also enable serde/derive Fix: the serialize feature should also enable serde/derive Sep 7, 2022
@Mingun
Copy link
Collaborator

Mingun commented Sep 7, 2022 •
edited
Loading

I would like instead derive Serialize/Deserialize only if serde/derive feature was activated in a downstream crate

@Mingun Mingun added enhancement serde Issues related to mapping from Rust types to XML labels Sep 7, 2022
@loyd
Copy link
Author

loyd commented Sep 8, 2022 •
edited
Loading

@Mingun, how is it possible? Also, I think it's the wrong behavior. A downstream crate can use impl Serialize directly and wants to use quick-xml. The need to explicitly provide a feature is exposing implementation details. The library can easily switch to impl Serialize instead of deriving and it should not change features.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2022

Codecov Report

Merging #472 (d921a58) into master (f8b292b) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   51.01%   51.00%   -0.02%     
==========================================
  Files          30       30              
  Lines       13103    13103              
==========================================
- Hits         6685     6683       -2     
- Misses       6418     6420       +2
Flag Coverage Δ
unittests 51.00% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/reader/parser.rs 98.57% <0.00%> (-0.72%) ⬇️
src/reader/buffered_reader.rs 85.09% <0.00%> (-0.49%) ⬇️
src/de/map.rs 71.62% <0.00%> (-0.46%) ⬇️
src/de/mod.rs 82.02% <0.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Mingun
Copy link
Collaborator

Mingun commented Sep 8, 2022

I don't known what steps exactly need to be done to achieve that. That needs an investigation. It is possible to enable feature of another crate, and since Rust 1.60 it is possible to enable feature, but does not automatically depend on that crate using "package-name?/feature-name" syntax. Maybe that may help. If not, that need to introduce another feature in quick-xml, something like "serializable-types" (better name welcomed) and enable serialization of QName & Co. only when this feature is enabled (instead of serialize).

A downstream crate can use impl Serialize directly and wants to use quick-xml. The need to explicitly provide a feature is exposing implementation details. The library can easily switch to impl Serialize instead of deriving and it should not change features.

If downstream crate implement traits manually, it won't needed derive. If it at the same time does not need to serialize QName, it shouldn't need the serde/derive in quick-xml

@loyd
Copy link
Author

loyd commented Sep 8, 2022

Ok, just fix it any way you want

@loyd loyd closed this Sep 8, 2022
@loyd
Copy link
Author

loyd commented Sep 8, 2022

Btw, a usual name for the feature is serde, because deserialization is also enabled.

@loyd loyd mentioned this pull request Sep 8, 2022
Closed
@Mingun
Copy link
Collaborator

Mingun commented Sep 8, 2022 •
edited
Loading

Btw, a usual name for the feature is serde, because deserialization is also enabled.

You're totally right and I even was thinking about rename that feature, but after that PR I think that this could be wrong. Usually feature named serde means, that crate has some types that implements Serialize and Deserialize and enabling that feature will enable them.

But in quick-xml serialize feature has other meaning: it enables XML (de)serializer. From that point of view enable Serialize and Deserialize for QName under that feature is wrong

@loyd
Copy link
Author

loyd commented Sep 8, 2022

You're right. Btw, another way is to use a separate crate (e.g. rmp-serde).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement serde Issues related to mapping from Rust types to XML

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@loyd @Mingun @codecov-commenter