Honor default values for null optional struct fields. Closes #85

[Gnyblast]

Implementing respecting to the default values for optional struct fields when they are null. This closes https://github.com/kubkon/zig-yaml/issues/85

[Gnyblast]

@kubkon any chance this get merged?

[kubkon]

Oh, and could you also include a test that covers this functionality please?

[Gnyblast]

Hi @kubkon,

Thanks for your valuable review, I implemented the change you offered and also added a test case.

[tazend]

Hi,

IMO it would be nice to have this also for non-optional fields in a nested struct.

Right now, the following example doesn't work and fails with error.StructfieldMissing if uid_range isn't in the yaml file being parsed:

pub const Config = struct {
    uid_range: struct {
         min: u32 = 1000,
         max: u32 = 100000,
    } = .{},
}

However, it would make sense to succeed here, since all values have default values. It already works for a "flat" struct like this

pub const Config = struct {
    port: u16 = 1000,
}

So for nested structs it should also work, and has the benefit that no unnecessary optional-checking has to be performed if there are well defined defaults.

Applying the following diff to the current PR makes this work (simply by moving out the retrieval of the defaultValue for the field before the if statement):

diff --git a/src/Yaml.zig b/src/Yaml.zig
index 0b06a05..1062332 100644
--- a/src/Yaml.zig
+++ b/src/Yaml.zig
@@ -180,11 +180,12 @@ fn parseStruct(self: Yaml, arena: Allocator, comptime T: type, map: Map) Error!T
             break :blk map.get(field_name);
         };
 
+        if (value == null) blk: {
+            const maybe_default_value = field.defaultValue() orelse break :blk;
+            value = Value.encode(arena, maybe_default_value) catch break :blk;
+        }
+
         if (@typeInfo(field.type) == .optional) {
-            if (value == null) blk: {
-                const maybe_default_value = field.defaultValue() orelse break :blk;
-                value = Value.encode(arena, maybe_default_value) catch break :blk;
-            }
             @field(parsed, field.name) = try self.parseOptional(arena, field.type, value);
             continue;
         }

From my quick tests, I haven't noticed any issues and everything seems to work as expected.

[Gnyblast]

Well it looks OK to me too since as you tested and found no side-effects. But the last word is gonna be said by project owner.

[kubkon]

Hi,

IMO it would be nice to have this also for non-optional fields in a nested struct.

Right now, the following example doesn't work and fails with error.StructfieldMissing if uid_range isn't in the yaml file being parsed:

pub const Config = struct {
    uid_range: struct {
         min: u32 = 1000,
         max: u32 = 100000,
    } = .{},
}

However, it would make sense to succeed here, since all values have default values. It already works for a "flat" struct like this

pub const Config = struct {
    port: u16 = 1000,
}

So for nested structs it should also work, and has the benefit that no unnecessary optional-checking has to be performed if there are well defined defaults.

Applying the following diff to the current PR makes this work (simply by moving out the retrieval of the defaultValue for the field before the if statement):

diff --git a/src/Yaml.zig b/src/Yaml.zig
index 0b06a05..1062332 100644
--- a/src/Yaml.zig
+++ b/src/Yaml.zig
@@ -180,11 +180,12 @@ fn parseStruct(self: Yaml, arena: Allocator, comptime T: type, map: Map) Error!T
             break :blk map.get(field_name);
         };
 
+        if (value == null) blk: {
+            const maybe_default_value = field.defaultValue() orelse break :blk;
+            value = Value.encode(arena, maybe_default_value) catch break :blk;
+        }
+
         if (@typeInfo(field.type) == .optional) {
-            if (value == null) blk: {
-                const maybe_default_value = field.defaultValue() orelse break :blk;
-                value = Value.encode(arena, maybe_default_value) catch break :blk;
-            }
             @field(parsed, field.name) = try self.parseOptional(arena, field.type, value);
             continue;
         }

From my quick tests, I haven't noticed any issues and everything seems to work as expected.

I am happy with landing this change but let's land it in a separate PR after this one is merged.