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

impl Serialize for Tweet/TwitterUser #99

Merged
merged 15 commits into from
Jul 10, 2020
Merged

impl Serialize for Tweet/TwitterUser #99

merged 15 commits into from
Jul 10, 2020

Conversation

QuietMisdreavus
Copy link
Collaborator

Closes #90

This PR adds a Serialize impl for various types in the library, most notably Tweet and TwitterUser. Since those types have existing Deserialize impls that load via a "raw" type that mirrors Twitter's JSON representation, i needed to introduce a new mechanism to allow loading via either Twitter's JSON or egg-mode's.

The way i did this for this PR was to introduce a new round_trip! macro that wrapped the structs and added some new items used internally by the macro:

  • SerCopy, a field-for-field copy of the given type,
  • SerEnum, an enum which can either be the "raw" type or SerCopy,
  • and a couple derived Deserialize implementations that leverage the #[serde(untagged)] attribute to drive the deserialization.

One major drawback with this approach is that if the "raw" type gets out-of-sync with Twitter's representation, the only error message you see is "data did not match any variant of untagged enum SerEnum", instead of knowing which field is missing. On the other hand, doing it this way means that there is no overhead of doing the deserialization manually (anyone remember the original FromJson impls?) or loading the whole response into a serde_json::Value first. I'm posting this provisionally to see if this suits people's purposes, and to ask whether this should wrap other public types in egg-mode.

the idea behind the macro is that i want the Deserialize impl to work
either with the original Twitter JSON, *or* with the serialization
output from the derived Serialize impl. Since i can't attempt an
automatic serialization directly into Tweet as-is (it would try to read
it as Twitter's JSON) i decided to make a copy of the struct definition
and provide a transparent From impl for easy conversion. combine this
with an enum labeled with `#[serde(untagged)]` and serde will do the
"try either of these" logic for me!

i had to leave out Place and User because they had complicated
serialization logic that i wanted to parcel out to other commits (and in
User's case, since it also contains a Tweet, i would have to apply
Serialize to both of them at the same time for it to "fully" work).
@QuietMisdreavus
Copy link
Collaborator Author

I had an idea about the error-message problem and posted a new commit. It adds two new functions to types that deserialize via round_trip!: twitter_deser_error and roundtrip_deser_error. They both attempt to deserialize into the inner types of SerEnum, and reports the error that comes out, if any. It's not a perfect solution, but should allow for some kind of error reporting when combined with the raw module. I'm not sure how to make it so that these can be called from the actual deserialization process, but it's at least something.

Copy link
Collaborator Author

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Some review notes to self while reading this over, two weeks later:

  • refactor idea: turn the error-reporting methods into a trait so i can write docs on how to pull a deser error?
    • even if i don't move the deserialization-error functions, i really want more docs somewhere to describe what you can do if something fails to load. i could throw something on the impl block, but that would either mean having the same sample on both tweets and users (which is awkward), or extending the macro to take in that sample (which is more awkward). at least in a trait i can either put multiple samples, or just say "this is for users, but tweets would work the same". 🤷‍♀️
  • i'd love to have some way to default the serde(untagged) error message to one of the variants (in this case the raw struct), but that's not a thing :/

where
S: Serializer,
{
ser.collect_str(&src.format("%a %b %d %T %z %Y"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tiny nit: might as well combine this date format string into a const

src/place/mod.rs Outdated
@@ -367,3 +368,39 @@ where
serde_json::from_value::<Vec<(f64, f64)>>(inner_arr).map_err(|e| D::Error::custom(e))
})
}

fn serialize_bounding_box<S>(src: &Vec<(f64, f64)>, ser: S) -> Result<S::Ok, S::Error>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit: should this combine (de)serialize_bounding_box into a single serialize module so you can combine the serde attributes into a single #[serde(with)]? probably, yeah

src/tweet/raw.rs Outdated
@@ -2,15 +2,17 @@ use crate::{place, user};
use chrono;
use serde::Deserialize;

use crate::common::{serde_datetime};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should probably trim the brackets here

@QuietMisdreavus QuietMisdreavus merged commit cad008b into master Jul 10, 2020
@QuietMisdreavus QuietMisdreavus deleted the ser branch July 10, 2020 16:39
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.

Serialize for Tweet?
1 participant