Use named arguments for JSON/YAML serialization #6448

Sija · 2018-07-26T13:07:58Z

luislavena · 2018-07-26T13:21:47Z

src/yaml/mapping.cr

@@ -120,7 +120,7 @@ module YAML
    #
    # FIXME: remove the dummy argument if we ever fix this.

-    def initialize(ctx : YAML::ParseContext, node : ::YAML::Nodes::Node, _dummy : Nil)
+    def initialize(ctx : YAML::ParseContext, node : ::YAML::Nodes::Node)


Shouldn't this use named arguments too?

luislavena · 2018-07-26T13:54:03Z

This might also need updates:

crystal/src/yaml/from_yaml.cr

Line 88 in 4f720ab

yield T.new(ctx, value)

https://github.com/crystal-lang/crystal/blob/master/src/yaml/from_yaml.cr#L113

https://github.com/crystal-lang/crystal/blob/master/src/yaml/from_yaml.cr#L129

Sija · 2018-07-26T14:34:29Z

Hmmm, I dunno what to make out of this error:

Error in spec/std_spec.cr:2: while requiring "./std/**"

require "./std/**"
^

in spec/std/yaml/serializable_spec.cr:771: instantiating 'YAMLAttrModuleTest2:Class#from_yaml(String)'

    it { YAMLAttrModuleTest2.from_yaml(%({"phoo": 20, "bar": 30})).to_tuple.should eq({10, 20, 30}) }
                             ^~~~~~~~~

in src/yaml/from_yaml.cr:2: read before assignment to local variable ''

  new(ctx: YAML::ParseContext.new, node: parse_yaml(string_or_io))
  ^~~

Sija · 2018-07-27T11:50:27Z

/cc @asterite

asterite · 2018-07-27T13:14:39Z

No idea, just stick with code that compiles.

kostya · 2018-07-27T16:30:31Z

why for json you change 2 lines, but for yaml 100500 lines.

this seems to work, specs and failed example compiles:

diff --git a/src/json/serialization.cr b/src/json/serialization.cr
index 46f0d3144..2b1050a1b 100644
--- a/src/json/serialization.cr
+++ b/src/json/serialization.cr
@@ -117,7 +117,7 @@ module JSON

       def self.new(pull : ::JSON::PullParser)
         instance = allocate
-        instance.initialize(pull, nil)
+        instance.initialize(pull: pull)
         GC.add_finalizer(instance) if instance.responds_to?(:finalize)
         instance
       end
@@ -132,7 +132,7 @@ module JSON
       end
     end

-    def initialize(pull : ::JSON::PullParser, dummy : Nil)
+    def initialize(*, pull : ::JSON::PullParser)
       {% begin %}
         {% properties = {} of Nil => Nil %}
         {% for ivar in @type.instance_vars %}
diff --git a/src/yaml/serialization.cr b/src/yaml/serialization.cr
index 1db40b557..c369e1441 100644
--- a/src/yaml/serialization.cr
+++ b/src/yaml/serialization.cr
@@ -123,7 +123,7 @@ module YAML

         ctx.record_anchor(node, instance)

-        instance.initialize(ctx, node, nil)
+        instance.initialize(ctx: ctx, node: node)
         GC.add_finalizer(instance) if instance.responds_to?(:finalize)
         instance
       end
@@ -138,7 +138,7 @@ module YAML
       end
     end

-    def initialize(ctx : YAML::ParseContext, node : ::YAML::Nodes::Node, dummy : Nil)
+    def initialize(*, ctx : YAML::ParseContext, node : ::YAML::Nodes::Node)
       {% begin %}
         {% properties = {} of Nil => Nil %}
         {% for ivar in @type.instance_vars %}

Sija · 2018-07-27T16:34:06Z

@kostya AFAIK CI complains but please go ahead and open a new PR to try it out.

straight-shoota approved these changes Jul 26, 2018

View reviewed changes

luislavena reviewed Jul 26, 2018

View reviewed changes

Sija force-pushed the fix-6405 branch 2 times, most recently from 371719c to 3156dce Compare July 26, 2018 13:29

Sija force-pushed the fix-6405 branch 3 times, most recently from a0e94ad to 4c60a1d Compare July 26, 2018 14:20

Use named arguments for JSON/YAML serialization

3af5c27

Sija force-pushed the fix-6405 branch from 4c60a1d to 3af5c27 Compare July 26, 2018 14:27

Sija closed this Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use named arguments for JSON/YAML serialization #6448

Use named arguments for JSON/YAML serialization #6448

Sija commented Jul 26, 2018

luislavena Jul 26, 2018

luislavena commented Jul 26, 2018

Sija commented Jul 26, 2018

Sija commented Jul 27, 2018

asterite commented Jul 27, 2018

kostya commented Jul 27, 2018 •

edited

Sija commented Jul 27, 2018

Use named arguments for JSON/YAML serialization #6448

Use named arguments for JSON/YAML serialization #6448

Conversation

Sija commented Jul 26, 2018

luislavena Jul 26, 2018

Choose a reason for hiding this comment

luislavena commented Jul 26, 2018

Sija commented Jul 26, 2018

Sija commented Jul 27, 2018

asterite commented Jul 27, 2018

kostya commented Jul 27, 2018 • edited

Sija commented Jul 27, 2018

kostya commented Jul 27, 2018 •

edited