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

serde serialize_with support #735

Merged
merged 5 commits into from Mar 4, 2024
Merged

serde serialize_with support #735

merged 5 commits into from Mar 4, 2024

Conversation

dakaizou
Copy link
Contributor

@dakaizou dakaizou commented Feb 4, 2024

I'm submitting a feature

Description

Adds the ability to serialize a Uuid as a Simple/Urn/Braced. Uuid::parse_str deserialize them correctly, but Uuid is always serialized as Hyphenated. Users would be able to decide which format to serialize to after this PR.

Example:

#[derive(serde_derive::Serialize)]
struct Struct {
    // This will be serialize as uuid::fmt::Simple
    #[serde(serialize_with = "uuid::serde::simple")]
    id: uuid::Uuid,
}

Tests

3 new tests for the feature were added.

crate::external::serde_support::serde_tests::test_serialize_as_simple
crate::external::serde_support::serde_tests::test_serialize_as_braced
crate::external::serde_support::serde_tests::test_serialize_as_urn

Related Issue(s)

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Nice! This looks good to me @dakaizou.

I don't think we need to do deserialize_with support, since the parser will very quickly identify the appropriate format to use. We can also add deserialize support in a non-breaking way in the future if we want to.

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Actually, having another look at the existing compact module, I think we should follow the same pattern here. Instead of top-level functions in uuid::serde, let's use modules with a serialize and deserialize function in them.

@dakaizou
Copy link
Contributor Author

dakaizou commented Feb 5, 2024

... I don't think we need to do deserialize_with support, since the parser will very quickly identify the appropriate format to use. ...

This was exactly what I was thinking, since they can all be deserialized with Uuid::deserialize, I only created functions for serialization.

I think we should follow the same pattern here. Instead of top-level functions in uuid::serde, let's use modules with a serialize and deserialize function in them.

compact::deserialize only deserializes [u8; 16], so it makes sense to put them in a mod for #[serde(with="compact")].
For Simple/Braced/Urn, do you think it's better to follow the same pattern? (e.g. rejecting the Braced format from Simple's deserialize)

If not, I will just proxy the function call:

pub fn deserialize<'de, D>(deserializer: D) -> Result<crate::Uuid, D::Error>
where
    D: serde::Deserializer<'de>,
{
    serde::Deserialize::deserialize(deserializer)
}

@KodrAus
Copy link
Member

KodrAus commented Feb 15, 2024

For Simple/Braced/Urn, do you think it's better to follow the same pattern? (e.g. rejecting the Braced format from Simple's deserialize)

I think we should only parse exactly the format requested. It looks like we don't have exposed methods for specific parsers, but we could make them pub(crate) and use those for these methods. I think it'll be more performant and align better with expectations that way.

@dakaizou
Copy link
Contributor Author

dakaizou commented Feb 18, 2024

  • change the return type of parse_simple and parse_hyphenated, so InvalidUuid is returned instead of ()
  • create two more methods in parser: parse_braced, parsed_urn
  • fix the error returned by visit_seq in Uuid::deserialize
  • create simple, braced, urn mod in serde_support

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @dakaizou! This is some great work 🙂

@KodrAus KodrAus merged commit 8c6fc53 into uuid-rs:main Mar 4, 2024
21 checks passed
mergify bot pushed a commit to andrzejressel/pulumi-wasm that referenced this pull request Mar 19, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [uuid](https://togithub.com/uuid-rs/uuid) | workspace.dependencies | minor | `1.7.0` -> `1.8.0` |

---

### Release Notes

<details>
<summary>uuid-rs/uuid (uuid)</summary>

### [`v1.8.0`](https://togithub.com/uuid-rs/uuid/releases/tag/1.8.0)

[Compare Source](https://togithub.com/uuid-rs/uuid/compare/1.7.0...1.8.0)

#### ⚠️ Potential Breakage ⚠️

A new `impl AsRef<Uuid> for Uuid` bound has been added, which can break inference on code like:

```rust
let b = uuid.as_ref();
```

You can fix these by explicitly typing the result of the conversion:

```rust
let b: &[u8] = uuid.as_ref();
```

or by calling `as_bytes` instead:

```rust
let b = uuid.as_bytes();
```

#### What's Changed

-   docs: fix small spelling mistake by [@&#8203;bengsparks](https://togithub.com/bengsparks) in [uuid-rs/uuid#737
-   serde serialize_with support by [@&#8203;dakaizou](https://togithub.com/dakaizou) in [uuid-rs/uuid#735
-   Fix up CI builds by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [uuid-rs/uuid#744
-   Only add `wasm-bindgen` as a dependency on `wasm32-unknown-unknown` by [@&#8203;emilk](https://togithub.com/emilk) in [uuid-rs/uuid#738
-   impl AsRef<Uuid> for Uuid by [@&#8203;koshell](https://togithub.com/koshell) in [uuid-rs/uuid#743
-   Add v6 to v8 draft link to README by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [uuid-rs/uuid#746
-   Add a workflow for running cargo outdated by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [uuid-rs/uuid#745
-   Prepare for 1.8.0 release by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [uuid-rs/uuid#747

#### New Contributors

-   [@&#8203;bengsparks](https://togithub.com/bengsparks) made their first contribution in [uuid-rs/uuid#737
-   [@&#8203;dakaizou](https://togithub.com/dakaizou) made their first contribution in [uuid-rs/uuid#735
-   [@&#8203;emilk](https://togithub.com/emilk) made their first contribution in [uuid-rs/uuid#738
-   [@&#8203;koshell](https://togithub.com/koshell) made their first contribution in [uuid-rs/uuid#743

**Full Changelog**: uuid-rs/uuid@1.7.0...1.8.0

</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.

---

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

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/andrzejressel/pulumi-wasm).
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Mar 19, 2024
Bumps uuid from 1.7.0 to 1.8.0.

Release notes
Sourced from uuid's releases.

1.8.0
⚠️ Potential Breakage ⚠️
A new impl AsRef<Uuid> for Uuid bound has been added, which can break inference on code like:
let b = uuid.as_ref();
You can fix these by explicitly typing the result of the conversion:
let b: &[u8] = uuid.as_ref();
or by calling as_bytes instead:
let b = uuid.as_bytes();
What's Changed

docs: fix small spelling mistake by @​bengsparks in uuid-rs/uuid#737
serde serialize_with support by @​dakaizou in uuid-rs/uuid#735
Fix up CI builds by @​KodrAus in uuid-rs/uuid#744
Only add wasm-bindgen as a dependency on wasm32-unknown-unknown by @​emilk in uuid-rs/uuid#738
impl AsRef for Uuid by @​koshell in uuid-rs/uuid#743
Add v6 to v8 draft link to README by @​KodrAus in uuid-rs/uuid#746
Add a workflow for running cargo outdated by @​KodrAus in uuid-rs/uuid#745
Prepare for 1.8.0 release by @​KodrAus in uuid-rs/uuid#747

New Contributors

@​bengsparks made their first contribution in uuid-rs/uuid#737
@​dakaizou made their first contribution in uuid-rs/uuid#735
@​emilk made their first contribution in uuid-rs/uuid#738
@​koshell made their first contribution in uuid-rs/uuid#743

Full Changelog: uuid-rs/uuid@1.7.0...1.8.0



Commits

0f2aaae Merge pull request #747 from uuid-rs/cargo/1.8.0
01d16c3 prepare for 1.8.0 release
e4746bc Merge pull request #745 from uuid-rs/ci/outdated
d0396ad Merge pull request #746 from uuid-rs/chore/draft-link
9415ed4 Merge pull request #743 from koshell/main
951e8e3 Merge pull request #738 from rerun-io/emilk/wasm-bindgen-only-on-web
101aa84 add v6 to v8 draft link to README
84dcbba run outdated on a schedule
ca952b1 add a workflow for running cargo outdated
abe995a Make the toml longer, more complicated, and functional
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
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

2 participants