search

egg-mode-rs / egg-mode

a twitter api crate for rust
https://crates.io/crates/egg-mode
Mozilla Public License 2.0
371 stars 65 forks source link

Serde integration #40

Closed adwhit closed 5 years ago

adwhit commented 6 years ago

Closes https://github.com/QuietMisdreavus/twitter-rs/issues/11

Since this PR has become massive, thought I ought to put it out there for feedback. Most of the LOC increase is from added tests, however. The overall logic should see a nice simplification. Will post more comments shortly.

adwhit commented 6 years ago
adwhit commented 6 years ago

I think it is ready for an initial review now. Happy for this to be seen as a 'starting point', don't expect it to be accepted wholesale. (I mostly did it to get more experience with the serde library).

QuietMisdreavus commented 6 years ago

Hey! I'm gradually getting back in the head-space for egg-mode, so i want to get this PR merged before i do anything big. I'm looking through the changes right now. Thanks so much for doing this!

It is not easy to integrate the codepoints_to_bytes functionality with serde. The reason being that now the deserialization of certain fields depends on the contents of other fields. To get this to work required a good bit of custom logic. It might be cleaner just to add a few helper methods e.g. tweeter.display_range() gets the correct slice, calling codepoints_to_bytes

I've considered doing this. If i provide something like this i would probably hide the indices entirely, just to prevent confusion. I'll have to take another look at where it's used, to make sure it wouldn't create too much burden.

I have refactored the tests and added lots more sample payloads. However quite a lot of the codebase is still untested/underspecified.

Thank you so much for doing this! It's probably the biggest weakness of egg-mode, that i don't have a good way to properly verify any of the parsing stuff, and make sure it stays up-to-date.

I have used ? quite extensively. For consistency, these should be converted back to try!, or vice versa

When i started egg-mode, ? wasn't available, and when it was introduced i was hesitant to start using it so i could keep some backwards-compatibility. However, ? is now really old in comparison to some of the other features that other libraries use all the time (pub(restricted), field-init shorthand, custom derive) so it's not a huge deal to start introducing it. Fully converting egg-mode to use ? can be deferred to another PR.

for UrlEntity, rather than writing a custom derive to populate possibly-missing display_url and expanded_url, I just marked those fields as #[serde(default)]

This can work out. It's a little awkward, but it can be warned against in documentation.

adwhit commented 6 years ago

Hi, thanks for the feedback! I do intend to do the work necessary to get it merged, but I just wanted to mention that it might be a few weeks until I get the chance to re-familiarize myself with the codebase enough to address the comments.

QuietMisdreavus commented 5 years ago

Since a new issue has cropped up that requires another major update to egg-mode (upgrading hyper), i went ahead and pushed the PR across the finish line. 1.18.0 failed, but it's failed before, and i needed to bump that anyway. Otherwise, i'll wait for Appveyor to finish and merge this. Thanks so much for writing this, and so sorry for letting it sit for so long!