Skip to content

Conversation

little-arhat
Copy link
Contributor

Many modern web APIs return their data as stream of (sometimes newline
delimited) JSON values. This commits add handy function parse_stream
that returns Iterator over parsed values.

This feature has some drawbacks or issues:

  • I'm not sure, whether it's good idea to expose JSONStream type, but it seems you cannot currently return abstract Iterator
  • Ideally, we should accept io::Read, but I couldn't get it to work, unfortunately: expected 'Iter', foundstd::io::Bytes`
  • The hardest one — naming, — not sure, how to name iterator and helper function.

Commit contains tests (and they are passing!)

WDYT?

Many modern web APIs return their data as stream of (sometimes newline
delimited) JSON values. This commits add handy function `parse_stream`
that returns Iterator over parsed values.
@little-arhat
Copy link
Contributor Author

should I be worried about failing build on stable? I currently can't even build stable myself:

/Users/u/.multirust/toolchains/stable/cargo/registry/src/github.com-0a35038f75765ae4/quasi-0.3.10/src/lib.rs:11:43: 11:66 error: #[feature] may not be used on the stable release channel
/Users/u/.multirust/toolchains/stable/cargo/registry/src/github.com-0a35038f75765ae4/quasi-0.3.10/src/lib.rs:11 #![cfg_attr(not(feature = "with-syntex"), feature(rustc_private))]

@little-arhat
Copy link
Contributor Author

@erickt how does it look? should I fix it for stable? I can't compile crate for stable on my machine (maybe should do some clean), but error from travis ci is error: use of unstable library feature 'core': the libcore library has not yet been scrutinized for stabilization in terms of structure and naming. And as far as I know, one can't simply use feature flags on stable.

@erickt
Copy link
Member

erickt commented Jan 10, 2016

@little-arhat: Sorry for the delay! I'll review it now.

json/src/de.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

You should use use std::marker::PhantomData here.

 * `parse_stream` removed in favor of direct `JSONStream` usage;
 * truncated input no longer silently ignored;
 * added test for errors;
 * style violation fixed.
@little-arhat
Copy link
Contributor Author

@erickt thanks, fixed all remarks.

@little-arhat
Copy link
Contributor Author

@erickt hey, did you have time to look at updated code? Thanks!

@little-arhat
Copy link
Contributor Author

Fixed some corner cases found during using this.

Also, "All checks have passed" — @erickt what do you think? %)

json/src/de.rs Outdated
let _:Result<()> = self.deser.parse_whitespace();
// Since Deserializer.eof() always return Ok(_), it's safe to
// call .ok() here
if self.deser.eof().ok().unwrap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Deserializer.eof does not always return Ok there is a try! in there.

.eof() can return Err, so do not try to unwrap() it unconditionally.

Also, check result of .parse_whitespaces(), and report, if there is any
errors.

(thanks, @nixpulvis)
@little-arhat
Copy link
Contributor Author

@nixpulvis you're right, Ok(try! has confused me! :) Thanks, fixed.

@little-arhat
Copy link
Contributor Author

@erickt hi, any updates on that? %)

@erickt
Copy link
Member

erickt commented Feb 23, 2016

@little-arhat: Sorry, I've been swamped with trying to get serde 0.7 out the door. That's almost across the line so I'm going to all these outstanding PRs again!

@erickt erickt merged commit 32f643f into serde-rs:master Feb 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants