-
-
Notifications
You must be signed in to change notification settings - Fork 39
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 default value instead of None for all types when using skip attribute #110
Conversation
I am not quite sure if this is correct - |
I understand that this is not serde, but my defense will be that: serde works like this use {
serde::{Deserialize, Serialize},
nanoserde::{SerJson, DeJson}, // Using nanoserde with PR's changes
};
#[derive(SerJson, DeJson, Serialize, Deserialize, Debug, Default)]
pub struct Test {
#[nserde(skip)]
#[serde(skip)]
pub end_frame: bool,
keyframes: i32,
}
fn main() {
let data = r#"{"animated": true, "keyframes": 1}"#;
let ns_test: Test = nanoserde::DeJson::deserialize_json(&data).expect("nanoserde cannot deserialize data");
dbg!(&ns_test);
let s_test: Test = serde_json::from_str(data).expect("serde cannot deserialize data");
dbg!(&s_test);
let ns_serialized = ns_test.serialize_json();
dbg!(ns_serialized);
let s_serialized = serde_json::to_string(&s_test).expect("serde cannot serialize");
dbg!(s_serialized);
} Output:
And if this is not ok will it be ok for me to add:
If this is possible of course to return error for certain attribute, or just show which attribute is wrong at least for derive DeJson macro
|
I was wrong and you are right it seems.
|
Just as a side note, you have to pass a function (not a value) in serde to use a custom default. I'm okay with this, it probably is better and should be backwards compatible. Would like to see a test of it being used on a non-optional though (and changed for ron) |
I agree, test would be nice. |
Did these things:
|
I liked |
I think using default instead of a None was a good thing. It would be nice to get some tests as we discussed earlier, but overall it was a good PR! This default function thing,I am not quite convienced about. |
Sorry So I left changes:
|
Yeah I was just saying that's how |
…ault Also tests and added attributes to feature table
CI looks unhappy, but apart from that looks good to me! |
Looks good to me also, thanks @birhburh |
When I was moving some stuff from serde to nanoserde it was little confusing why I get None value for f32 fields in macro expansion
I think this is the solution for issue #107