Skip to content

Conversation

jwilm
Copy link
Contributor

@jwilm jwilm commented Feb 2, 2016

NOT READY this needs changes before merging; Cargo.toml spec is hard coded to a git rev

Mappings from visit_* to de/serialize_* should be self explanatory. [serde(deny_unknown_fields)] was added to a test struct since it was checking for denial. A couple of error types were added to support new
serde de::value::Error variants. Finally, there are a bunch of throwaway changes pinning the serde version to a 0.7 commit. These should be removed once 0.7 lands in favor of a crates.io semver spec.

@jwilm
Copy link
Contributor Author

jwilm commented Feb 2, 2016

Hmm, tests passed on my machine. Will look into the failure later.

@jwilm
Copy link
Contributor Author

jwilm commented Feb 6, 2016

The build is now passing. The serde dependency will still need to be updated to 0.7 once it is published.

@jwilm
Copy link
Contributor Author

jwilm commented Feb 10, 2016

Tests are passing with latest master of serde

ErrorCode::MissingField(ref field) => write!(f, "missing field \"{}\"", field),
ErrorCode::TrailingCharacters => "trailing characters".fmt(f),
ErrorCode::UnexpectedEndOfHexEscape => "unexpected end of hex escape".fmt(f),
ErrorCode::Length(ref len) => write!(f, "incorrect value length {}", len),

Choose a reason for hiding this comment

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

These changes are already contained in PR #11 and are likely to conflict if that PR is merged.

@jwilm
Copy link
Contributor Author

jwilm commented Feb 23, 2016

Rebased on latest json#master and tested with latest serde#master. There's one test failing right now that I don't have time to chase down; It looks like the deny_unknown_fields attribute isn't working as expected. Link to failing line.

@tomprogrammer
Copy link

I also stumbled across that issue. Look at this https://github.com/tomprogrammer/json/commit/5eb789dc4d8ad4db013b3f8f9881b6009a50d69c commit. You can see I introduced a new ErrorCode::UnkownVariant. You can use it like

 ("{\"unknown\":[]}", Error::Syntax(ErrorCode::UnknownVariant("unknown".to_string()), 1, 10)),

@tomprogrammer
Copy link

The interaction between your and my PR are a bit unfortunate though. I don't know how to solve this best.

@jwilm
Copy link
Contributor Author

jwilm commented Feb 23, 2016

@erickt just mentioned

On serde_json, I've also got 0.7 almost merged in.

So my branch may be moot. Still, any commit adding support for 0.7 will have similar conflicts.

@erickt
Copy link
Member

erickt commented Feb 27, 2016

This has actually landed since serde 0.7 was released!

@erickt erickt closed this 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