Added support for mapping YAML nodes not specified in properties #4411

aaronbronow · 2017-05-12T23:48:39Z

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 · 2017-05-12T23:49:36Z

To Do:

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

aaronbronow · 2017-05-12T23:51:46Z

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 · 2017-05-13T13:55:40Z

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 · 2017-05-13T14:33:37Z

No automated test for this?

bew · 2017-05-13T14:38:58Z

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

…roperty name

aaronbronow · 2017-05-13T18:32:32Z

{% 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 · 2017-05-13T18:37:51Z

src/yaml/mapping.cr

    def initialize(%pull : ::YAML::PullParser)
      {% for key, value in properties %}
        %var{key.id} = nil
        %found{key.id} = false
      {% end %}

+      {% if extra %}
+        {{extra.id}} = Hash(String, ::YAML::Any).new


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

aaronbronow · 2017-05-14T19:58:23Z

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 · 2017-05-14T21:33:34Z

src/yaml/mapping.cr

@@ -181,6 +181,14 @@ module YAML
            {% end %}
          end
        {% end %}
+        {% if extra %}
+          _{{extra.id}} = @{{extra.id}}


I'd suggest using % fresh variables.

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

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}}.

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 · 2017-09-23T22:55:49Z

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

kostya · 2018-04-23T18:52:02Z

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

straight-shoota · 2018-07-02T19:22:41Z

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

Added support for mapping YAML nodes not specified in properties

73627b4

forgot to format

af9520f

mverzilli added kind:feature pr:needs-review topic:stdlib labels May 13, 2017

aaronbronow added 2 commits May 13, 2017 11:08

Added macro exception when extra property name collides with mapped p…

c30e25c

…roperty name

Added test for extra attribute mapping in YAML parser

ad4a54c

bew reviewed May 13, 2017

View reviewed changes

aaronbronow added 2 commits May 14, 2017 11:25

Removed unneeded local variable assignment

f8c23b9

Added extra mapping to to_yaml

5d4a021

forgot to format

f98ddb5

Sija reviewed May 14, 2017

View reviewed changes

aaronbronow added 2 commits September 23, 2017 09:26

Merge remote-tracking branch 'upstream/master'

09ed51e

Added support for extras in JSON mapping

d63aa62

kostya mentioned this pull request Apr 25, 2018

add json mapping extra fields #6009

Closed

RX14 closed this Jul 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for mapping YAML nodes not specified in properties #4411

Added support for mapping YAML nodes not specified in properties #4411

aaronbronow commented May 12, 2017

aaronbronow commented May 12, 2017 •

edited

aaronbronow commented May 12, 2017

bew commented May 13, 2017 •

edited

dmitryrck commented May 13, 2017

bew commented May 13, 2017

aaronbronow commented May 13, 2017

bew May 13, 2017

aaronbronow commented May 14, 2017

Sija May 14, 2017

aaronbronow May 15, 2017

bcardiff May 15, 2017

aaronbronow May 15, 2017 •

edited

akzhan commented Sep 23, 2017

kostya commented Apr 23, 2018 •

edited

straight-shoota commented Jul 2, 2018

Added support for mapping YAML nodes not specified in properties #4411

Added support for mapping YAML nodes not specified in properties #4411

Conversation

aaronbronow commented May 12, 2017

aaronbronow commented May 12, 2017 • edited

aaronbronow commented May 12, 2017

bew commented May 13, 2017 • edited

dmitryrck commented May 13, 2017

bew commented May 13, 2017

aaronbronow commented May 13, 2017

bew May 13, 2017

Choose a reason for hiding this comment

aaronbronow commented May 14, 2017

Sija May 14, 2017

Choose a reason for hiding this comment

aaronbronow May 15, 2017

Choose a reason for hiding this comment

bcardiff May 15, 2017

Choose a reason for hiding this comment

aaronbronow May 15, 2017 • edited

Choose a reason for hiding this comment

akzhan commented Sep 23, 2017

kostya commented Apr 23, 2018 • edited

straight-shoota commented Jul 2, 2018

aaronbronow commented May 12, 2017 •

edited

bew commented May 13, 2017 •

edited

aaronbronow May 15, 2017 •

edited

kostya commented Apr 23, 2018 •

edited