Add Serializable #6

danini-the-panini · 2025-01-31T09:06:21Z

resolves #2

danini-the-panini · 2025-02-21T09:41:25Z

@BigBoyBarney ok I've got an initial working solution, do you mind playing around with it and let me know if it works? One limitation is to_kdl returns a single node with the name of the class, not sure how else to turn it into a document

BigBoyBarney · 2025-02-22T10:58:45Z

@danini-the-panini thank you for your hard work!

I've been playing around with it for a bit, and got it working with varying degrees of success.

I think the underlying API is pretty reasonable, very creative usage of annotations! I left a few comments on the code!

BigBoyBarney · 2025-02-22T12:00:54Z

The following document

doc = KDL.parse <<-KDL
    Collected-Pokemon {
      Snorlax "The Sleepy" status=asleep pokemon-level=10
      Jigglypuff "The Cute" status=awake pokemon-level=1
    }

    Trainers {
      trainer Sylphrena
      trainer DaniniThePanini
    }
    KDL

can be parsed with

class Info
  include KDL::Serializable

  @[KDL::Child(name: "Collected-Pokemon")]
  property collected_pokemon : Pokemon

  @[KDL::Child(name: "Trainers")]
  property trainers : Trainers
end

class Pokemon
  include KDL::Serializable

  @[KDL::Child(name: "Snorlax")]
  property snorlax : PokemonInfo

  @[KDL::Child(name: "Jigglypuff")]
  property jigglypuff : PokemonInfo
end

class PokemonInfo
  include KDL::Serializable

  @[KDL::Argument]
  property nick : String

  @[KDL::Property]
  property status : String

  @[KDL::Property(name: "pokemon-level")]
  property level : Int32
end

class Trainers
  include KDL::Serializable

  @[KDL::Children(name: "trainer")]
  property trainers : Array(Trainer)
end

class Trainer
  include KDL::Serializable

  @[KDL::Argument]
  property name : String
end

obj = Info.from_kdl doc

Which is really cool! However, as you can see, to get the actual array of trainers, one would need to call obj.trainers.trainers. Is it possible to somehow unwrap a child node and its children, so that I could do

  @[KDL::Child(name: "Trainers", unwrap: "children", children_name: "trainer")]
  property trainers : Array(String)

Or something along these lines. :D

BigBoyBarney · 2025-02-22T11:00:43Z

spec/kdl/serializable_spec.cr

+  def initialize(@first, @second, @numbers, @foo, @bar, @map, @arg, @args, @props, @norf, @things)
+  end


This is technically not necessary and can be omitted altogether :D

BigBoyBarney · 2025-02-22T11:02:43Z

src/kdl/serializable.cr

+          {% elsif value[:unwrap] == "properties" %}
+            %var{name} = node.child({{name.stringify}}).properties.transform_values { |v, _| convert(v.value, {{ value[:type].type_vars[1] }}) }
+          {% else %}
+            %var{name} = {{value[:type]}}.from_kdl(node.child({{name.stringify}}))


- %var{name} = {{value[:type]}}.from_kdl(node.child({{name.stringify}})) + %var{name} = {{value[:type]}}.from_kdl(node.child({{value[:name]}}))

We have to use value[:name] otherwise the name property will have no effect in certain cases.

BigBoyBarney · 2025-02-22T11:05:14Z

src/kdl/serializable.cr

+                nilable:     ivar.type.nilable?,
+                type:        ivar.type
+              }
+              all_properties[ivar.id] = other_children[ivar.id]


other_children doesn't exist, so if the code gets here, it will throw a compile time error 100% of the time (which happens whenever a property has no annotation)

BigBoyBarney · 2025-02-22T16:52:41Z

I just discovered that including KDL::Serializable makes it impossible to new up an instance, even if it has default values.

class DB
  include KDL::Serializable

  @[KDL::Argument(name: "Version")]
  property version = 1

  @[KDL::Argument(name: "Path")]
  property path = "~/.local/share/anime_database.db"
end


 43 | property db = DB.new
                       ^--
Error: wrong number of arguments for 'Shisho::Util::DB.new' (given 0, expected 1)

Overloads are:
 - Shisho::Util::DB.new(node : ::KDL::Node)
 - Shisho::Util::DB.new(doc : ::KDL::Document)

This can be circumvented by defining an empty initialize method.

BigBoyBarney · 2025-02-22T17:01:46Z

src/kdl/serializable.cr

+        {% for name, value in child_annos %}
+          # child
+          {% if value[:unwrap] == "argument" %}
+            %var{name} = convert(node.arg({{name.stringify}}), {{ value[:type] }})


Same as below

BigBoyBarney · 2025-02-22T17:01:51Z

src/kdl/serializable.cr

+          {% if value[:unwrap] == "argument" %}
+            %var{name} = convert(node.arg({{name.stringify}}), {{ value[:type] }})
+          {% elsif value[:unwrap] == "arguments" %}
+            %var{name} = node.args({{name.stringify}}).map { |v| convert(v, {{ value[:type].type_vars[0] }}) }


Same as below

BigBoyBarney · 2025-02-22T17:02:00Z

src/kdl/serializable.cr

+          {% elsif value[:unwrap] == "arguments" %}
+            %var{name} = node.args({{name.stringify}}).map { |v| convert(v, {{ value[:type].type_vars[0] }}) }
+          {% elsif value[:unwrap] == "properties" %}
+            %var{name} = node.child({{name.stringify}}).properties.transform_values { |v, _| convert(v.value, {{ value[:type].type_vars[1] }}) }


Same as below

BigBoyBarney · 2025-02-22T17:11:30Z

spec/kdl/serializable_spec.cr

+    obj.things[0].value.should eq "foo"
+    obj.things[1].value.should eq "bar"
+    obj.things[2].value.should eq "baz"


This syntax is quite common, e.g.:

some-paths { path "path/to/something" path "path/to/something/else" }

Having to call #value on it is a bit cumbersome. Directly newing up Array(String) for such cases would be nicer. This is related to my other comment about arrays in serializable.cr :D

BigBoyBarney · 2025-02-22T17:13:37Z

src/kdl/serializable.cr

+def Object.from_kdl(doc)
+  new doc
+end


This should not be global, but should be in the module instead, as we can only call #from_kdl on Types that include ::KDL::Serializable

BigBoyBarney · 2025-02-22T17:29:54Z

src/kdl/serializable.cr

+def Array.from_kdl(node : ::KDL::Node)
+  node.arguments.map do |arg|
+    arg.value
+  end
+end


I don't think this is needed?

BigBoyBarney · 2025-02-22T17:31:11Z

src/kdl/serializable.cr

+                  type:        ivar.type,
+                  converter:   ann[:converter],
+                  presence:    ann[:presence]
+                }


unwrap: ann[:unwrap]

together with the change further down at #children enables child array unwrapping

BigBoyBarney · 2025-02-22T17:32:27Z

src/kdl/serializable.cr

+          %var{name} = node.children.select { |n| n.name == {{value[:name]}} }.map { |n| {{value[:type].type_vars[0]}}.from_kdl(n) }
+          %found{name} = true


replace with

# children {% if value[:unwrap] == "argument" %} %var{name} = node.children.select { |n| n.name == {{value[:name]}} }.map { |n| convert(n.arguments[0].value, {{value[:type].type_vars[0]}}) } {% else %} %var{name} = node.children.select { |n| n.name == {{value[:name]}} }.map { |n| {{value[:type].type_vars[0]}}.from_kdl(n) } {% end %} %found{name} = true

To allow argument unwrapping for example. Can be extended to allow properties and multiples arguments too

BigBoyBarney · 2025-02-22T17:46:40Z

Code comments are getting a bit out of hand, especially without permission to propose changes, so I propose that you merge whatever you feel like merging, and I'll PR the suggested changes :D

danini-the-panini · 2025-02-24T17:11:46Z

@BigBoyBarney I've added your suggestions, and I've add a new unwrap: "dash_vals" that lets you do e.g.

things {
  - "foo"
  - "bar"
  - "baz"
}

@[KDL::Child(unwrap: "dash_vals")
property things : Array(String)

BigBoyBarney · 2025-02-24T19:16:03Z

Looks overall good to me!
Arbitrary children + argument and children + property unwrapping is missing, but I will PR that once this is merged. I already have it implemented locally as I need it for my app's config.

This would effectively be @[KDL::Children] alternative to @[KDL::Child(unwrap: "dash_vals") and and allow for any unwrapping.

As an example, see provisional KDL config file and the corresponding class

The only other thing worth mentioning is my 2nd to last comment in serializable.cr:

def Object.from_kdl(doc)
  new doc
end

should not be defined globally. This has to be in the included macro, otherwise this defines a #from_kdl method on all types, everywhere.

danini-the-panini · 2025-02-25T07:39:57Z

Ok, I've added more of the unwrap options you suggested

BigBoyBarney · 2025-02-25T08:50:38Z

Looks good to me! Parses my app's config file out of the box now.

Excellent work, thank you for your time and effort!

BigBoyBarney · 2025-02-25T08:55:22Z

src/kdl/serializable.cr

+      def self.new(node : ::KDL::Node)
+        new_from_kdl_node(node)
+      end
+
+      def self.new(doc : ::KDL::Document)
+        new_from_kdl_node(::KDL::Node.new("__root", children: doc))
+      end


An additional initialize method would be needed to allow newing up fully defined classes / structs.

def initialize end

Otherwise something like

class Mappings include KDL::Serializable @[KDL::Property(name: "update-frequency")] property update_frequency = 1 @[KDL::Property(name: "last-update")] property last_update = 0 end

will complain that Mappings.new needs an argument.

Error: wrong number of arguments for 'Shisho::Util::Mappings.new' (given 0, expected 1) Overloads are: - Shisho::Util::DB.new(node : ::KDL::Node) - Shisho::Util::DB.new(doc : ::KDL::Document)

Actually this breaks the spec, so will have to figure something else out 🤔

~~Yeah, we can't have a default initializer unless all properties have a default value. Alternatively, we ditch the new and just do everything through from_kdl~~

Just realiased this has to go through new anyway because it is constructing an object.

Users of Serializable will just have to add their own explicit constructors then

Any way would work for me :D Currently I'm manually defining a default initializer in all fully defined classes to make it compile, but an automatic solution would be cool

I'll see what I can do. If all ivars have default values I can auto-define a default initializer

ok not sure how to get it to work unfortunately :/

I was incorrectly under the assumption that ::JSON::Serializable did it automatically. It does not and requires a manual def initialize; end as well.

Never mind then :D

If you ask me, I think the PR is ready to be merged in that case. Excellent work! 👏

Thanks for all the help!

danini-the-panini added the enhancement New feature or request label Jan 31, 2025

danini-the-panini self-assigned this Jan 31, 2025

danini-the-panini mentioned this pull request Jan 31, 2025

Serializable implementation #2

Closed

attempt to add serializable annotations

0991900

danini-the-panini force-pushed the serializable branch from 1ec5642 to 0991900 Compare February 20, 2025 14:57

danini-the-panini added 2 commits February 20, 2025 17:48

made some progress

e283df0

it's working!

646f627

BigBoyBarney suggested changes Feb 22, 2025

View reviewed changes

BigBoyBarney reviewed Feb 22, 2025

View reviewed changes

BigBoyBarney suggested changes Feb 22, 2025

View reviewed changes

add unwrap children and dash_vals

5fa44cc

danini-the-panini added 2 commits February 25, 2025 08:38

remove global monkey patches

45dcedf

add more unwrap serializable options

0d1689d

BigBoyBarney suggested changes Feb 25, 2025

View reviewed changes

danini-the-panini merged commit b1b2a9e into main Feb 25, 2025
1 check passed

danini-the-panini deleted the serializable branch February 25, 2025 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Serializable #6

Add Serializable #6

danini-the-panini commented Jan 31, 2025

danini-the-panini commented Feb 21, 2025

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025 •

edited

Loading

BigBoyBarney Feb 22, 2025 •

edited

Loading

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney Feb 22, 2025

BigBoyBarney commented Feb 22, 2025

danini-the-panini commented Feb 24, 2025 •

edited

Loading

BigBoyBarney commented Feb 24, 2025 •

edited

Loading

danini-the-panini commented Feb 25, 2025

BigBoyBarney commented Feb 25, 2025

BigBoyBarney Feb 25, 2025 •

edited

Loading

BigBoyBarney Feb 25, 2025

danini-the-panini Feb 25, 2025 •

edited

Loading

BigBoyBarney Feb 25, 2025

danini-the-panini Feb 25, 2025

danini-the-panini Feb 25, 2025

BigBoyBarney Feb 25, 2025 •

edited

Loading

danini-the-panini Feb 25, 2025

		def initialize(@first, @second, @numbers, @foo, @bar, @map, @arg, @args, @props, @norf, @things)
		end

		%var{name} = node.children.select { \|n\| n.name == {{value[:name]}} }.map { \|n\| {{value[:type].type_vars[0]}}.from_kdl(n) }
		%found{name} = true

Add Serializable #6

Add Serializable #6

Conversation

danini-the-panini commented Jan 31, 2025

danini-the-panini commented Feb 21, 2025

BigBoyBarney commented Feb 22, 2025 • edited Loading

BigBoyBarney commented Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

BigBoyBarney Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

BigBoyBarney Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

BigBoyBarney commented Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigBoyBarney commented Feb 22, 2025

danini-the-panini commented Feb 24, 2025 • edited Loading

BigBoyBarney commented Feb 24, 2025 • edited Loading

danini-the-panini commented Feb 25, 2025

BigBoyBarney commented Feb 25, 2025

BigBoyBarney Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danini-the-panini Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigBoyBarney Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

BigBoyBarney Feb 22, 2025 •

edited

Loading

BigBoyBarney Feb 22, 2025 •

edited

Loading

BigBoyBarney commented Feb 22, 2025 •

edited

Loading

danini-the-panini commented Feb 24, 2025 •

edited

Loading

BigBoyBarney commented Feb 24, 2025 •

edited

Loading

BigBoyBarney Feb 25, 2025 •

edited

Loading

danini-the-panini Feb 25, 2025 •

edited

Loading

BigBoyBarney Feb 25, 2025 •

edited

Loading