search

mild-times / webmanifest

Create a webmanifest file
Apache License 2.0
19 stars 2 forks source link

Derive Deserialize for Manifest #2

Closed maxdeviant closed 6 years ago

maxdeviant commented 6 years ago

Choose one: this is a 🙋 feature

This PR derives Deserialize for Manifest.

Had to change the icons and related_applications collections to own their data, but the change should be transparent to consumers.

Checklist

Context

Closes #1.

Semver Changes

Minor

maxdeviant commented 6 years ago

@yoshuawuyts I changed the two problematic collections to own their data instead of holding references and then used clone in the builder to avoid changing the public API.

If you'd rather we change that instead to avoid a clone, let me know.

yoshuawuyts commented 6 years ago

Ohhh, this is perfect! Hadn't even thought of using clone here! Thanks so much!

yoshuawuyts commented 6 years ago

Pushed this version!

Hmm, something that's not clear to me still is how we could implement a Manifest::from_str() method. I reckon most people that want to deserialize are probably reading a file from disk.

For example this doesn't work:


  /// Create an instance from a string.
  pub fn from_str(input: &'s str) -> Result<Self, Error> {
    let manifest: Self = serde_json::from_str(input)?;
    Ok(manifest)
  }

@maxdeviant any thoughts?