support for arbitrary maps and sequences #129
Conversation
|
I ran exactly on this error yesterday, and I can confirm your fix works for me. Good to merge imho, thanks :) |
|
Hi @punkstarman any chance you look at it? thanks |
| @@ -16,3 +16,5 @@ thiserror = "1.0" | |||
| serde_derive = "1.0" | |||
| simple_logger = "1.0.1" | |||
| docmatic = "0.1.2" | |||
| serde_json = "1.0" | |||
| serde-transcode = "1.1.0" | |||
There was a problem hiding this comment.
I would rather not have these dependencies, even if they are only used for testing.
I believe that the same behaviour can be specified and tested in the next larger context.
| @@ -150,6 +150,7 @@ fn test_doctype() { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[ignore] | |||
There was a problem hiding this comment.
Why is this test ignored?
I suggest that
- either it should be made to pass
- or it should be modified because the behaviour is impacted by the change
- or it should be removed with an explanation as to why the behaviour cannot be changed without taking it beyond recognition
There was a problem hiding this comment.
it's failing man, try tunning it on master
| @@ -1,6 +1,7 @@ | |||
| extern crate docmatic; | |||
|
|
|||
| #[test] | |||
| #[ignore] | |||
There was a problem hiding this comment.
Again, tests should not be ignored in a pull request.
There was a problem hiding this comment.
it's failing on master branch, try it
| serde_transcode::transcode(deserializer, &mut serializer)?; | ||
| Ok(String::from_utf8(out)?) | ||
| } | ||
| fn _to_json<'a>(deserializer: impl Deserializer<'a>) -> Result<String, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
I do not agree with this style of preceding a function name with an underscore in this context.
I suggest removing the initial underscore from the function name.
| @@ -159,3 +161,24 @@ fn collection_of_enums() { | |||
| } | |||
| ); | |||
| } | |||
|
|
|||
| fn _to_xml<'a>(deserializer: impl Deserializer<'a>) -> Result<String, Box<dyn std::error::Error>> { | |||
There was a problem hiding this comment.
I do not agree with this style of preceding a function name with an underscore in this context.
I suggest removing the initial underscore from the function name.
|
|
||
| // fn is_human_readable(&self) -> bool { | ||
| // false | ||
| // } |
There was a problem hiding this comment.
Never submit commented code in a pull request.
Please remove this.
| let mut deserializer = serde_json::de::Deserializer::from_str(example_json); | ||
| let xml = _to_xml(&mut deserializer).expect("failed to transcode json into xml"); | ||
| assert_eq!(r##"<map><field fieldName="Asdasd"><list><item>asdadsda</item></list></field></map>"##.to_string(), xml) | ||
| } |
There was a problem hiding this comment.
Again, I would rather not rely on serde_json or serde_transcode to specify and test the behaviour introduced by this pull request. I believe that the same goal can be accomplished by considering a slightly more general context. This kind of test does have value for checking a specific outcome quickly, but I don't believe that they belong in the final version.
I suggest simply adding a test for the list case, similar to test_serialize_simple_map.
This is needed for serde-transcode to work properly when encoding an arbitrary serde-deserializable format into xml using your crate. Serde formats are supposed to support that in any shape or form according the serde docs https://serde.rs/transcode.html
Previous behaviour was to just panic.