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 proof of concept serde support with PathBuf #512

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jamesmunns
Copy link

This is a proof of concept for supporting types from async_std to be used with Serde. For now I have only implemented support for PathBuf, but if you like this, we can start rolling it out to all of the wrapped types.

// src/lib.rs:
use async_std;
use serde::{self, Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
struct Hmmm {
    ex: async_std::path::PathBuf,
}

fn main() {
    println!("Hello, world!");
}
# Cargo.toml
[package]
name = "serde-ex"
version = "0.1.0"
authors = ["James Munns <[email protected]>"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies.async-std]
version = "1.0"
path = "../async-std"

[dependencies.serde]
features = ["derive"]
version = "1.0"

@yoshuawuyts
Copy link
Contributor

@jamesmunns would it be possible to use serde-derive here instead so that we can call the feature serde?

@jamesmunns
Copy link
Author

@yoshuawuyts I think so! It's a habit of mine to just use serde as a dependency, but it's likely not needed here, just serde-derive.

@jamesmunns
Copy link
Author

Things that need to be decided now:

  1. What should the feature name be? just serde if we can depend on serde-derive? Something else?
  2. Should this be behind the unstable feature? e.g. require serde-support AND unstable be set for now?
  3. Are there any cases where a simple flatten won't work? Maybe wrapper types with more than one field?

Once we decide and correct those, we can probably land this with just PathBuf supported, then start incrementally covering more data types. Or we can hit them all at once in this PR.

@yoshuawuyts
Copy link
Contributor

Another thing we may want to consider is adding an entry to our "features" section in lib.rs: https://docs.rs/async-std/1.0.0/async_std/#features. This seems like something people may want to be aware of.

Cargo.toml Outdated Show resolved Hide resolved
@jamesmunns
Copy link
Author

@yoshuawuyts ah, no, I think we need a serde dependency as well. Otherwise I get:

    Checking async-std v1.0.0 (/tmp/async-std)
error[E0463]: can't find crate for `serde`
  --> /tmp/async-std/src/path/pathbuf.rs:22:49
   |
22 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
   |                                                 ^^^^^^^^^^^ can't find crate

@ghost
Copy link

ghost commented Nov 12, 2019

Should we put serde support behind a new feature flag, perhaps named serde1?

See also: rust-lang/api-guidelines#180

@jamesmunns
Copy link
Author

Hmm, this needs a deeper look. My first approach won't work.

use async_std;
use serde_json::to_string;
use serde::{self, Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
struct Hmmm {
    ex: async_std::path::PathBuf,
}

fn main() {
    dbg!(to_string(&Hmmm {
        ex: async_std::path::PathBuf::new(),
    }).unwrap());
}
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("can only flatten structs and maps (got a string)", line: 0, column: 0)', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

There's likely still a way, just probably a bit less elegant. Merits further investigation.

@jamesmunns
Copy link
Author

But it does work for newtypes! c70f00e switches PathBuf to be a newtype, rather than a struct containing an inner field.

[src/main.rs:11] to_string(&Hmmm{ex: async_std::path::PathBuf::new(),}).unwrap() = "{\"ex\":\"\"}"

I don't think this needs to be a breaking change? And for fields where a newtype doesn't work, we can always manually impl Serialize/Deserialize

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Nov 14, 2019

I don't think this needs to be a breaking change?

We've never provided guarantees about internals, so indeed don't think it's a breaking change.

I think this PR looks great; but probably want to give @stjepang a chance to sign off on it too before we merge.

@yoshuawuyts yoshuawuyts added this to the 1.1.0 milestone Nov 20, 2019
@jamesmunns
Copy link
Author

@stjepang just let me know if you'd prefer serde or serde1 as the feature flag, then I think we can land this, and start adding serde support to other structures.

@yoshuawuyts yoshuawuyts requested a review from a user November 25, 2019 19:39
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@yoshuawuyts and I discussed this during triage.

I'm 👍 with the idea of making more and more async-std types implement Serialize and Deserialize.

I don't think those implementations should also go behind the unstable flag, that would a bit too cautious to the point where stabilizations becomes an nuisance with little benefit.

Approving. Thanks for submitting the PR! :)

@ghost
Copy link

ghost commented Dec 13, 2019

@dtolnay Do you think we should name the feature flag serde or serde1?

Copy link

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I recommend sticking with "serde".

Two thoughts:

  • I am skeptical of making this a default feature. What fraction of users need this enabled? Or is it default only because you already depend on something that depends on serde?
  • I am skeptical of using serde_derive. A pair of Serialize/Deserialize impls for a wrapper type that forwards to the inner PathBuf's impls are ~15 lines to write. Is that a better tradeoff?

@yoshuawuyts
Copy link
Contributor

I am skeptical of making this a default feature. (...)

Agreed; this should only be available when the serde feature is enabled. We currently don't depend on serde anywhere else, so it's indeed a new dep.

I am skeptical of using serde_derive. (...)

Oh, yeah a manual impl def sounds better here. Thanks for suggesting!

@maisiliym
Copy link

maisiliym commented Oct 20, 2020

Status?
Is anyone working on a PR to convert all wrapper types to newtypes, with serde feature?

Edit: That approach wont work in every case, as quick experiment with Path showed.
Indeed, manual impls will be needed.
The case has been brought to serde: serde-rs/serde#1913

@hoijui
Copy link

hoijui commented Jun 2, 2024

How is the state of this?
As I understood from the conversation here, it is ready to be merged.. no?

( personally need serde support exactly for PathBuf)

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.

5 participants