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

Add documentation for tagged enums with Serde #594

Merged
merged 1 commit into from Jun 12, 2023
Merged

Add documentation for tagged enums with Serde #594

merged 1 commit into from Jun 12, 2023

Conversation

Kriskras99
Copy link
Contributor

As discussed in #586 here is some documentation for working with tagged enums.
I changed my implementation to a macro because it became tedious very fast (I need to deserialize an enum with 70 variants).

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

Looks good! Can you also add a changelog entry

I also try to play with it in next days, because right now doctests are definitely will fail, because macro and usage in different blocks. We can merge both blocks or move macro implementation to it's own file and include! it in both blocks

src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
@Kriskras99 Kriskras99 closed this Apr 21, 2023
@Kriskras99 Kriskras99 reopened this Apr 21, 2023
@Kriskras99
Copy link
Contributor Author

I've added a changelog entry and merged the two blocks. I've also included all your feedback except for the about using MapAccessDeserializer

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

Merging #594 (858118e) into master (88a2f27) will increase coverage by 0.04%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
+ Coverage   64.43%   64.48%   +0.04%     
==========================================
  Files          36       36              
  Lines       16919    16921       +2     
==========================================
+ Hits        10902    10911       +9     
+ Misses       6017     6010       -7     
Flag Coverage Δ
unittests 64.48% <0.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
src/de/mod.rs 69.28% <0.00%> (+0.26%) ⬆️
src/serde_helpers.rs 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

Changelog.md Outdated
@@ -15,7 +15,7 @@
### Bug Fixes

### Misc Changes

- [#594]: Add documentation for using tagged enums with Serde.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Link should be actually added:

Suggested change
- [#594]: Add documentation for using tagged enums with Serde.
- [#594]: Add documentation for using tagged enums with Serde.
[#594]: https://github.com/tafia/quick-xml/pull/594

Also check cargo fmt -- it seems that there are some trailing spaces!

@Mingun
Copy link
Collaborator

Mingun commented Apr 21, 2023

I played with the implementation a bit. The reason why you've failed with MapAccessDeserializer in that that you have incorrect expectations of how the XML should be deserialized.

The types

#[derive(Deserialize, Debug, PartialEq)]
pub struct Root {
    pub one: TaggedEnum,
    pub two: TaggedEnum,
}

#[derive(Deserialize, Debug, PartialEq)]
#[serde(tag = "@tag")]
pub enum TaggedEnum {
    Spam(Spam),
    Ham,
}

#[derive(Deserialize, Debug, PartialEq)]
pub struct Spam {
    #[serde(rename = "@ham")]
    pub ham: u64,
}

describes the following XML

<root>
    <one tag="spam" ham="42" />
    <two tag="ham" />
</root>

you can make sure if you mentally replace each enumeration variant with an equivalent type: see example.

I was able to deserialize it successfully using approach with MapAccessDeserializer.

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

I decided that since we now have a special module for helpers, add a macro there. That simple macro should help with typical tasks.

@dralley, @Kriskras99, if you have time could you please check grammar?

@Mingun Mingun merged commit 621da10 into tafia:master Jun 12, 2023
6 checks passed
@Kriskras99
Copy link
Contributor Author

@Mingun sorry I let this pull request sit. I have reviewed the documentation and did find some grammar issues, I'll open a PR to fix them!

crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 29, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [quick-xml](https://github.com/tafia/quick-xml) | dependencies | minor | `0.28.2` -> `0.29.0` |

---

### Release Notes

<details>
<summary>tafia/quick-xml</summary>

### [`v0.29.0`](https://github.com/tafia/quick-xml/blob/HEAD/Changelog.md#&#8203;0290----2023-06-13)

[Compare Source](tafia/quick-xml@v0.28.2...v0.29.0)

##### New Features

-   [#&#8203;601]: Add `serde_helper` module to the crate root with some useful utility
    functions and document using of enum's unit variants as a text content of element.
-   [#&#8203;606]: Implement indentation for `AsyncWrite` trait implementations.

##### Bug Fixes

-   [#&#8203;603]: Fix a regression from [#&#8203;581] that an XML comment or a processing
    instruction between a <!DOCTYPE> and the root element in the file brokes
    deserialization of structs by returning `DeError::ExpectedStart`
-   [#&#8203;608]: Return a new error `Error::EmptyDocType` on empty doctype instead
    of crashing because of a debug assertion.

##### Misc Changes

-   [#&#8203;594]: Add a helper macro to help deserialize internally tagged enums
    with Serde, which doesn't work out-of-box due to serde limitations.

[#&#8203;581]: tafia/quick-xml#581

[#&#8203;594]: tafia/quick-xml#594

[#&#8203;601]: tafia/quick-xml#601

[#&#8203;603]: tafia/quick-xml#603

[#&#8203;606]: tafia/quick-xml#606

[#&#8203;608]: tafia/quick-xml#608

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTguMCIsInVwZGF0ZWRJblZlciI6IjM1LjExOC4wIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1940
Reviewed-by: crapStone <crapstone01@gmail.com>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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

3 participants