Use default value instead of None for all types when using skip attribute

[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

[not-fl3]

I am not quite sure if this is correct - default attribute seems more appropriate for a default initialization, while Option looks like a good semantics for missing values?

[birhburh]

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:

[src/main.rs:17:5] &ns_test = Test {
    end_frame: false,
    keyframes: 1,
}
[src/main.rs:19:5] &s_test = Test {
    end_frame: false,
    keyframes: 1,
}
[src/main.rs:21:5] ns_serialized = "{\"keyframes\":1}"
[src/main.rs:23:5] s_serialized = "{\"keyframes\":1}"

And if this is not ok will it be ok for me to add:

1. more informative error message that skip only supports Option type (and also add skip to feature table in readme) Because now it looks like this:

   error[E0308]: mismatched types
   --> src/main.rs:6:19
   |
   6 | #[derive(SerJson, DeJson, Serialize, Deserialize, Debug, Default)]
   |                   ^^^^^^ expected `bool`, found `Option<_>`
   |
   = note: expected type `bool`
            found enum `std::option::Option<_>`
   = note: this error originates in the derive macro `DeJson` (in Nightly builds, run with -Z macro-backtrace for more info)

   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 Ok, just checked and lots of code should be modified to use proc_macro::Span, but I really like the idea

2. support of #[nserde(skip, default)] combination that uses default value or provided default value instead of None? It conforms to serde and also make more sense, I think Ok, just checked, using provided default value isn't working in serde: https://github.com/serde-rs/serde/issues/1030

[not-fl3]

I was wrong and you are right it seems.

skip - always skip and use defaults defaults - use defaults only when there is no value in json This makes sense and I believe your PR is doing exactly this. Merging this, @knickish ?

[knickish]

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)

[not-fl3]

I agree, test would be nice. Maybe asserting that value is present in the json, but is, indeed ignored If the value is not being present in the json it is still correctly default-initialized. And if both default and skip attributes are present that the defaults are from the attribute, not just None.

[birhburh]

Did these things:

• Removed default_with
• Made default accept function name(path) so it works as serde now
• Implemented skip attribute for ron format
• Added tests for json and ron for default and skip
• Added these attributes to feature table in readme

[not-fl3]

I liked #[nserde(default = 4.0)] a lot more than fn b_default() -> f32 {4.0} #[nserde(default = "b_default")]

[not-fl3]

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. Maybe there are some corner cases when having a function instead of a value may be benefitial, but I don't think its in a scope of this PR. And I do like to have default values right there in a struct definition instead of all those functions.

[birhburh]

Sorry Somehow I read that @knickish wanted this behavior, but he didn't

So I left changes:

• Removed default_with
• Made default accept function name(path) so it works as serde now
• Implemented skip attribute for ron format
• Added tests for json and ron for default, default_with and skip
• Added these attributes to feature table in readme

[knickish]

Yeah I was just saying that's how serde does it (presumably because writing the type definition directly would get unwieldy for large types). I'm ok with either way for the attribute

[not-fl3]

CI looks unhappy, but apart from that looks good to me!

[knickish]

Looks good to me also, thanks @birhburh