Added support for mapping YAML nodes not specified in properties

[aaronbronow]

This change is intended to provide access to YAML nodes not specified in YAML.mapping by setting them in a node named by the user.

It can be used with this code:

require "yaml"

yaml = <<-END
  name: "foo"
  children:
  - name: "bar"
  - name: "baz"
  unknown: true
  another_unknown: false
  END

class Foo
  YAML.mapping({
    name: String,
    children: Array(Hash(String,String))},
    strict: false,
    extra: "extras"
  )
end

foo = Foo.from_yaml(yaml)

pp foo
puts foo.to_yaml

# This generates: 
# foo # => #<Foo:0x102d97f00
#           @children=[{"name" => "bar"}, {"name" => "baz"}],
#           @extras={"unknown" => "true", "another_unknown" => "false"},
#           @name="foo">
# ---
# name: foo
# children:
# - name: bar
# - name: baz

[aaronbronow]

To Do:

• Update to_yaml to provide the extra hash
• Apply the same change to JSON.mapping
• Write tests

[aaronbronow]

Question:
What should happen when the extra value (the key of the new hash) is the same as one of the specified keys?

If extra: "name" it will conflict with the name key set in properties. Right now the result is instance variable '@name' of Foo must be Hash(String, YAML::Any), not String but that's not very clear.

[bew]

What should happen when the extra value (the key of the new hash) is the same as one of the specified keys?

I think you should check for this case in the beginning of the macro, and raise a custom error if it happens.
Something like the following could be used:

  {% if extra && properties.keys.includes? extra.id %}
    {% raise "The name '#{extra.id}' for extra is already used" %}
  {% end %}

[dmitryrck]

No automated test for this?

[bew]

@dmitryrck it's on his todolist (#4411 (comment))

[aaronbronow]

{% if extra && properties.keys.includes? extra.id %}
{% raise "The name '#{extra.id}' for extra is already used" %}
{% end %}

I implemented this suggestion and now it's a macro exception. I don't know how to write a spec for a macro exception so there's no test for that currently.

[bew]

Why not use directly @{{extra.id}} (and remove the @{{extra.id}} = {{extra.id}} at the end of the macro) ?

[aaronbronow]

to_yaml is updated to emit the extra hash if it's available.

Question: why create a temporary variable here instead of work with @{{key.id}} directly? I don't see why we need the temp variable.

{% for key, value in properties %}
          _{{key.id}} = @{{key.id}}

          unless _{{key.id}}.nil?
            # Key
            {{value[:key] || key.id.stringify}}.to_yaml(%yaml)

            # Value
            {% if value[:converter] %}
              {{ value[:converter] }}.to_yaml(_{{key.id}}, %yaml)
            {% else %}
              _{{key.id}}.to_yaml(%yaml)
            {% end %}
          end
        {% end %}
        {% if extra %}
          _{{extra.id}} = @{{extra.id}}
          
          unless _{{extra.id}}.nil?
            "{{extra.id}}".to_yaml(%yaml)
            _{{extra.id}}.to_yaml(%yaml)
          end
        {% end %}

[Sija]

I'd suggest using % fresh variables.

[aaronbronow]

Ok, but why not just call .to_yaml on the @ instance variable?

[bcardiff]

if @{{extra.id}} exists, then it is not nillable so you could call @{{extra.id}}.to_yaml(... directly. For memory footprint I would suggest to let it be lazy initialized / nilable. But for the use case, if the user expect to have some extras, maybe is good enough to eagerly create a and instance for @{{extra.id}}.

[aaronbronow]

In this implementation extra is nil unless the user has set it and passed it in as an argument to the mapping(...) macro. Because the macro checks for existence of extra before emitting code, @{{extra.id}} won't exist at all if the user is not using extras. If extra does exist then @{{extra.id}} will always get initialized as a Hash(String, ::YAML::Any)

If I'm understanding this correctly, the only problem with trying to call @{{extra.id}}.to_yaml(... would be if @{{extra.id}} is nil at the time of executing this block and that's impossible. I hope I'm understanding correctly 😄

[akzhan]

@aaronbronow Looks like #5007 will be merged soon. Please review your pull request to meet this breaking change.

[kostya]

Hm, i think this was already merged. Any chance to merge it before next release, at least for json?

[straight-shoota]

This can probably be closed due to JSON::Serializable (as was it successor #6009).