Actually this still doesn't handle the case where the type is both nested and nilable 🤔

          {% else %}
            %child = node.child?({{value[:name]}})
            %var{name} = if %child
                          {{value[:type]}}.from_kdl(%child)
                        else
                          {% if value[:has_default] %}
                            {{value[:default]}}
                          {% elsif value[:nilable] %}
                            nil
                          {% else %}
                            raise SerializableException.new("Missing argument for KDL node: {{name}}")
                          {% end %}
                        end


(MissingArgumentWithDefault | Nil).from_kdl(__temp_3430)

if the ivar is nilable, the type includes Nil and we can't call from_kdl on it 🤔

@BigBoyBarney can you not use TypeNode#union_types to extract the non-nilable type?

if %child
  {{(value[:type].union_types - [Nil]).first}}.from_kdl(%child)
else
  ...

of course we might have to loop through all union types and attempt .from_kdl on each of them to support other kinds of union types

I can check tonight but I don't think so because we need to cleanly handle the Nil type too.

I solved it in this commit by implementing from_kdl on Union(*T), which is also how the stdlib' JSON::Serializable does it.

Types that can be initalised from kdl should have the method already, and types that don't will result in a comptime error so it's pretty nice.

Credits to Hertzdevil for the idea.

I'll add a few more tests tonight to make sure I didn't miss anything and prettify the specs a bit.

Won't (Nil | String) always deserialise to nil?

Nope! :D

HOWEVER, it attempts to call #from_kdl on String, which obviously does not exist, so it fails at comp time. Unlucky. I thought I could get away with a cheeky blanket call to #from_kdl on non-nil types, but apparently not!

I'll make this PR a draft, I have a long day tomorrow but I can flesh this out over the weekend! :D Will probably have to introduce some kind of pull parser from KDL::Node, like how JSON::Serializable does it.

Sorry about the notification spam lately!

Can we not just extend String to have a to_kdl method?

We totally could! That would be the simplest solution for sure, but I thought maybe we could unify all the convert() stuff as well into a pull like how JSON does it, for elegance reasons.

Will have to play around with it a bit to see if there's a compelling reason for it though, if not, we can just define #from_kdl on all the basic types.

I think we should try pull first, since extending built-in types might be undesirable.

Nvm I was wrong the last time, JSON::Serializable extends the types too (https://github.com/crystal-lang/crystal/blob/d755f761d6d0d36f1a00827a5d71ba3fa191be41/src/json/from_json.cr), it just uses PullParser to make the code a bit more streamlined.

KDL's equivalent of a PullParser would essentially be the convert method in serializable.cr and the logic in the various branches of the macro.

Okay so turns out I was wrong again 🤣

Thankfully, we don't need to do any of that basic type extension just yet, as .from_kdl can never be called for them, since they will always have to be annotated with unwrap:.

This PR is now ready for review, I added a bunch of tests, too, so it should be fine (I hope).

This PR only implements the missing / default logic for Child class nodes and those with unwrap: argument. Further work is needed for children, properties etc.

Aparte:

I added a pending spec for the deserialize / serialize round trip because that was incorrectly passing on main. See #13 for more detail.