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

Move Token De/Serializer to serde_test crate #412

Merged
merged 11 commits into from
Jun 29, 2016
Merged

Move Token De/Serializer to serde_test crate #412

merged 11 commits into from
Jun 29, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jun 29, 2016

Fixes #411.

@dtolnay dtolnay changed the title Test Move Token De/Serializer to serde_test crate Jun 29, 2016
@dtolnay
Copy link
Member Author

dtolnay commented Jun 29, 2016

@oli-obk it would be best to review the commits individually - there are a few changes to make the API more presentable, especially around the Error variant and the assert_* functions.

This is required for linked_hash_map to drop its dependency on serde_json: https://github.com/dtolnay/linked-hash-map/commit/dabca618762ed31b50bfa5ea92b5bbfc305d57c5?diff=split which is required for fixing serde-rs/json#91.

@oli-obk
Copy link
Member

oli-obk commented Jun 29, 2016

Just out of curiosity. Is there a concrete motivation for this? (not a blocker at all, just curious)

Nevermind. The previous post has the motivation.

reviewing now

@@ -0,0 +1,14 @@
[package]
name = "serde_test"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Since people are actually meant to use this crate, maybe we should sync versions with serde. Otherwise this becomes a piston-like mess

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. Done.

}

fn cause(&self) -> Option<&error::Error> {
None
Copy link
Member

Choose a reason for hiding this comment

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

this was this way before the refactoring, but I wonder if it should forward to de::value::Error in case of ValueError?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got rid of ValueError in a later commit. Nothing was using ValueError.

@oli-obk
Copy link
Member

oli-obk commented Jun 29, 2016

lgtm

For a public crate, the docs are very sparse. Also I'm not sure if the docs will be included in the automatically generated ones hosted by @erickt .

Both issues can be addressed later.

@oli-obk oli-obk merged commit d1be5ef into serde-rs:master Jun 29, 2016
@dtolnay dtolnay deleted the test branch June 29, 2016 08:06
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.

2 participants