-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use custom format to serialize the json #156
Conversation
gostkin
commented
Dec 5, 2022
•
edited
Loading
edited
- Impl serialize
- Impl deserialize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general just some small comments.
|
||
impl Display for Value { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
f.serialize_str(self.clone().to_string().map_err(|err| std::fmt::Error::custom(format!("Can't serialize {:?}", err)))?.as_str()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a dumb question, but how does this work? We've got use std::fmt::{Display, Formatter};
and Display
uses that one, so where do we get serialize_str
from? I see it's in serde::fmt::Formatter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically if you can provide a string it will be interpreted / serialized as a string. so I use my to_string()
to convert to string and then use the standard serialize_str. Not sure if it's the best approach, but it should work at least
rust/src/json_serialize.rs
Outdated
for char in string.graphemes(true) { | ||
let (reset_string, token) = match char { | ||
"\"" => { | ||
if !current_string.is_empty() && current_string.clone().graphemes(true).last().clone().unwrap() == "\\" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the clones necessary here to call graphemes()
? The crate seems to take a reference although it's implemented on str
not String
.
ParsedValue { | ||
value: Value, | ||
}, | ||
ParsedKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment that this contains the :
token inside it I guess just to make it completely clear.
cml_arr.extend(vec![true, false].into_iter().map(|b| Value::Bool(b))); | ||
cml_arr.extend(vec![0, u64::MAX].into_iter().map(|integer| Value::Number(BigInt::from(integer)))); | ||
cml_arr.extend(vec![0, i64::MAX, i64::MIN].into_iter().map(|integer| Value::Number(BigInt::from(integer)))); | ||
cml_arr.extend(vec!["supported_string", ""].into_iter().map(|str| Value::String(String::from(str)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a unicode test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there're some
("\"y̆\\\"\"".to_string(), vec![JsonToken::Quote, JsonToken::String { raw: "y̆\"".to_string() }, JsonToken::Quote], Value::String("y̆\"".to_string())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have an idea for better test - pls tell
serde_arr | ||
} | ||
|
||
fn generate_array_of_primitives_with_unsupported() -> Vec<Value> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment might help to someone reading this later without knowing why we did this as to why it's called unsupported.
rust/src/json_serialize.rs
Outdated
generate_array(generate_objects()), | ||
generate_array(generate_arrays()), | ||
generate_object(generate_arrays()), | ||
generate_object(generate_arrays()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this meant to be generate_object(generate_objects()),
to cover all 4 mixes?
@@ -0,0 +1,996 @@ | |||
use std::collections::{BTreeMap, VecDeque}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to document why we have this instead of using the serde dependency that we use in other parts of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM