Repository: not-fl3/nanoserde
Pull request #110 (2024-08-17 15:59:42)

Use default value instead of None for all types when using skip attribute
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
```rs
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 

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 

Patch:
diff --git a/README.md b/README.md
index b16a57b..3e2f164 100644
--- a/README.md
+++ b/README.md
@@ -33,36 +33,39 @@ For more examples take a look at [tests](/tests)
 
 ## Features support matrix:
 
-| Feature                                        | json   | bin   | ron    | toml  |
-| ---------------------------------------------- | ------ | ----- | ------ | ----- |
-| serialization                                  | yes    | yes   | yes    | no    |
-| deserialization                                | yes    | yes   | yes    | no    |
-| container: Struct                              | yes    | yes   | yes    | no    |
-| container: Tuple Struct                        | no     | yes   | yes    | no    |
-| container: Enum                                | yes    | yes   | yes    | no    |
-| field: `std::collections::HashMap`             | yes    | yes   | yes    | no    |
-| field: `std::vec::Vec`                         | yes    | yes   | yes    | no    |
-| field: `Option`                                | yes    | yes   | yes    | no    |
-| field: `i*`/`f*`/`String`/`T: De*/Ser*`        | yes    | yes   | yes    | no    |
-| field attribute: `#[nserde(default)]`          | yes    | no    | yes    | no    |
-| field attribute: `#[nserde(rename = "")]`      | yes    | yes   | yes    | no    |
-| field attribute: `#[nserde(proxy = "")]`       | no     | yes   | no     | no    |
-| container attribute: `#[nserde(default)]`      | yes    | no    | yes    | no    |
-| container attribute: `#[nserde(rename = "")]`  | yes    | yes   | yes    | no    |
-| container attribute: `#[nserde(proxy = "")]`   | yes    | yes   | no     | no    |
-| container attribute: `#[nserde(transparent)]`  | yes    | no    | no     | no    |
+| Feature                                             | json   | bin   | ron    | toml  |
+| --------------------------------------------------- | ------ | ----- | ------ | ----- |
+| serialization                                       | yes    | yes   | yes    | no    |
+| deserialization                                     | yes    | yes   | yes    | no    |
+| container: Struct                                   | yes    | yes   | yes    | no    |
+| container: Tuple Struct                             | no     | yes   | yes    | no    |
+| container: Enum                                     | yes    | yes   | yes    | no    |
+| field: `std::collections::HashMap`                  | yes    | yes   | yes    | no    |
+| field: `std::vec::Vec`                              | yes    | yes   | yes    | no    |
+| field: `Option`                                     | yes    | yes   | yes    | no    |
+| field: `i*`/`f*`/`String`/`T: De*/Ser*`             | yes    | yes   | yes    | no    |
+| field attribute: `#[nserde(default)]`               | yes    | no    | yes    | no    |
+| field attribute: `#[nserde(rename = "")]`           | yes    | yes   | yes    | no    |
+| field attribute: `#[nserde(proxy = "")]`            | no     | yes   | no     | no    |
+| container attribute: `#[nserde(default)]`           | yes    | no    | yes    | no    |
+| container attribute: `#[nserde(default = "")]`      | yes    | no    | yes    | no    |
+| container attribute: `#[nserde(default_with = "")]` | yes    | no    | yes    | no    |
+| container attribute: `#[nserde(skip)]`              | yes    | no    | yes    | no    |
+| container attribute: `#[nserde(rename = "")]`       | yes    | yes   | yes    | no    |
+| container attribute: `#[nserde(proxy = "")]`        | yes    | yes   | no     | no    |
+| container attribute: `#[nserde(transparent)]`       | yes    | no    | no     | no    |
 
 ## Crate features:
 
-All features are enabled by default. To enable only specific formats, import nanoserde using 
+All features are enabled by default. To enable only specific formats, import nanoserde using
 ```toml
 nanoserde = { version = "*", default-features = false, features = ["std", "{format feature name}"] }
 ```
 in your `Cargo.toml` and add one or more of the following crate features:
 
-| Format    | Feature Name   | 
+| Format    | Feature Name   |
 | ----------| -------------- |
-| Binary    | `binary`       | 
-| JSON      | `json`         | 
+| Binary    | `binary`       |
+| JSON      | `json`         |
 | RON       | `ron`          |
 | TOML      | `toml`         |
diff --git a/derive/src/serde_json.rs b/derive/src/serde_json.rs
index 46ba989..0bedcd4 100644
--- a/derive/src/serde_json.rs
+++ b/derive/src/serde_json.rs
@@ -175,7 +175,7 @@ pub fn derive_de_json_named(name: &str, defaults: bool, fields: &[Field]) -> Tok
             matches.push((json_fieldname.clone(), localvar.clone()));
             local_vars.push(localvar);
         } else {
-            unwraps.push(format!("None"));
+            unwraps.push(default_val.unwrap_or_else(|| String::from("Default::default()")));
         }
 
         struct_field_names.push(struct_fieldname);
diff --git a/derive/src/serde_ron.rs b/derive/src/serde_ron.rs
index 48004eb..01d87e0 100644
--- a/derive/src/serde_ron.rs
+++ b/derive/src/serde_ron.rs
@@ -43,6 +43,10 @@ pub fn derive_ser_ron_struct(struct_: &Struct) -> TokenStream {
         let struct_fieldname = field.field_name.clone().unwrap();
         let ron_fieldname =
             shared::attrs_rename(&field.attributes).unwrap_or_else(|| struct_fieldname.clone());
+        let skip = shared::attrs_skip(&field.attributes);
+        if skip {
+            continue;
+        }
         if field.ty.base() == "Option" {
             l!(
                 s,
@@ -156,42 +160,47 @@ pub fn derive_de_ron_named(
         };
         let ron_fieldname =
             shared::attrs_rename(&field.attributes).unwrap_or(struct_fieldname.clone());
-
-        if field.ty.base() == "Option" {
-            unwraps.push(format!(
-                "{{
-                    if let Some(t) = {} {{
-                        t
-                    }} else {{
-                        {}
-                    }}
-                }}",
-                localvar,
-                default_val.unwrap_or_else(|| String::from("None"))
-            ));
-        } else if container_attr_default || default_val.is_some() {
-            unwraps.push(format!(
-                "{{
-                    if let Some(t) = {} {{
-                        t
-                    }} else {{
-                        {}
-                    }}
-                }}",
-                localvar,
-                default_val.unwrap_or_else(|| String::from("Default::default()"))
-            ));
+        let skip = crate::shared::attrs_skip(&field.attributes);
+
+        if skip == false {
+            if field.ty.base() == "Option" {
+                unwraps.push(format!(
+                    "{{
+                        if let Some(t) = {} {{
+                            t
+                        }} else {{
+                            {}
+                        }}
+                    }}",
+                    localvar,
+                    default_val.unwrap_or_else(|| String::from("None"))
+                ));
+            } else if container_attr_default || default_val.is_some() {
+                unwraps.push(format!(
+                    "{{
+                        if let Some(t) = {} {{
+                            t
+                        }} else {{
+                            {}
+                        }}
+                    }}",
+                    localvar,
+                    default_val.unwrap_or_else(|| String::from("Default::default()"))
+                ));
+            } else {
+                unwraps.push(format!(
+                    "{{
+                        if let Some(t) = {} {{
+                            t
+                        }} else {{
+                            return Err(s.err_nf(\"{}\"))
+                        }}
+                    }}",
+                    localvar, struct_fieldname
+                ));
+            }
         } else {
-            unwraps.push(format!(
-                "{{
-                    if let Some(t) = {} {{
-                        t
-                    }} else {{
-                        return Err(s.err_nf(\"{}\"))
-                    }}
-                }}",
-                localvar, struct_fieldname
-            ));
+            unwraps.push(default_val.unwrap_or_else(|| String::from("Default::default()")));
         }
 
         struct_field_names.push(struct_fieldname);
diff --git a/derive/src/shared.rs b/derive/src/shared.rs
index 735bf75..b826a17 100644
--- a/derive/src/shared.rs
+++ b/derive/src/shared.rs
@@ -69,7 +69,7 @@ pub fn attrs_transparent(attributes: &[crate::parse::Attribute]) -> bool {
         .any(|attr| attr.tokens.len() == 1 && attr.tokens[0] == "transparent")
 }
 
-#[cfg(feature = "json")]
+#[cfg(any(feature = "json", feature = "ron"))]
 pub fn attrs_skip(attributes: &[crate::parse::Attribute]) -> bool {
     attributes
         .iter()


Test patch:
diff --git a/tests/json.rs b/tests/json.rs
index a9a79e4..d8b0bc8 100644
--- a/tests/json.rs
+++ b/tests/json.rs
@@ -326,6 +326,71 @@ fn de_field_default() {
     assert_eq!(test.foo2.x, 3);
 }
 
+#[test]
+fn de_ser_field_skip() {
+    #[derive(DeJson, SerJson)]
+    struct Foo {
+        x: i32,
+    }
+    impl Default for Foo {
+        fn default() -> Foo {
+            Foo { x: 23 }
+        }
+    }
+
+    fn h_default() -> Option<String> {
+        Some("h not empty".into())
+    }
+
+    #[derive(DeJson, SerJson)]
+    pub struct Test {
+        a: i32,
+        #[nserde(skip)]
+        foo: Foo,
+        foo2: Foo,
+        #[nserde(skip, default = "4.0")]
+        b: f32,
+        #[nserde(skip, default)]
+        c: f32,
+        #[nserde(skip)]
+        d: i32,
+        #[nserde(skip)]
+        e: String,
+        #[nserde(skip)]
+        f: Foo,
+        #[nserde(skip)]
+        g: Option<i32>,
+        #[nserde(skip, default_with = "h_default")]
+        h: Option<String>,
+    }
+
+    let json = r#"{
+        "a": 1,
+        "c": 3.0,
+        "h": "h not empty",
+        "foo2": { "x": 3 }
+    }"#;
+
+    let mut test: Test = DeJson::deserialize_json(json).unwrap();
+    assert_eq!(test.a, 1);
+    assert_eq!(test.b, 4.0);
+    assert_eq!(test.c, 0.0);
+    assert_eq!(test.d, 0);
+    assert_eq!(test.e, "");
+    assert_eq!(test.f.x, 23);
+    assert_eq!(test.g, None);
+    assert_eq!(test.h, Some("h not empty".into()));
+    assert_eq!(test.foo.x, 23);
+    assert_eq!(test.foo2.x, 3);
+
+    test.e = "e not empty".into();
+    test.g = Some(2);
+
+    let ser_json = r#"{"a":1,"foo2":{"x":3}}"#;
+    let serialized = SerJson::serialize_json(&test);
+    assert_eq!(serialized, ser_json);
+}
+
 #[test]
 fn doctests() {
     /// This is test
diff --git a/tests/ron.rs b/tests/ron.rs
index 67a6e10..2706a95 100644
--- a/tests/ron.rs
+++ b/tests/ron.rs
@@ -91,6 +91,9 @@ fn de_field_default() {
             Foo { x: 23 }
         }
     }
+    fn foo3_default() -> Foo {
+        Foo { x: 15 }
+    }
 
     #[derive(DeRon)]
     pub struct Test {
@@ -98,6 +101,8 @@ fn de_field_default() {
         #[nserde(default)]
         foo: Foo,
         foo2: Foo,
+        #[nserde(default_with = "foo3_default")]
+        foo3: Foo,
         b: f32,
     }
 
@@ -112,6 +117,56 @@ fn de_field_default() {
     assert_eq!(test.b, 2.);
     assert_eq!(test.foo.x, 23);
     assert_eq!(test.foo2.x, 3);
+    assert_eq!(test.foo3.x, 15);
+}
+
+#[test]
+fn de_ser_field_skip() {
+    #[derive(DeRon, SerRon, PartialEq, Debug)]
+    struct Foo {
+        x: i32,
+    }
+    impl Default for Foo {
+        fn default() -> Foo {
+            Foo { x: 23 }
+        }
+    }
+    fn foo3_default() -> Foo {
+        Foo { x: 15 }
+    }
+
+    #[derive(DeRon, SerRon, PartialEq, Debug)]
+    pub struct Test {
+        a: i32,
+        #[nserde(skip)]
+        foo: Foo,
+        foo2: Foo,
+        #[nserde(skip, default_with = "foo3_default")]
+        foo3: Foo,
+        b: f32,
+        #[nserde(skip)]
+        c: Option<i32>,
+    }
+
+    let ron = r#"(
+        a: 1,
+        b: 2.,
+        foo2: (x: 3)
+    )"#;
+
+    let mut test: Test = DeRon::deserialize_ron(ron).unwrap();
+    assert_eq!(test.a, 1);
+    assert_eq!(test.b, 2.);
+    assert_eq!(test.foo.x, 23);
+    assert_eq!(test.foo2.x, 3);
+    assert_eq!(test.foo3.x, 15);
+
+    test.c = Some(2);
+    let serialized = SerRon::serialize_ron(&test);
+
+    let test: Test = DeRon::deserialize_ron(ron).unwrap();
+    let deserialized: Test = DeRon::deserialize_ron(&serialized).unwrap();
+    assert_eq!(deserialized, test);
 }
 
 #[test]