-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use named arguments for JSON/YAML serialization #6448
Conversation
src/yaml/mapping.cr
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use named arguments too?
371719c
to
3156dce
Compare
This might also need updates: Line 88 in 4f720ab
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 |
a0e94ad
to
4c60a1d
Compare
Hmmm, I dunno what to make out of this error:
|
/cc @asterite |
No idea, just stick with code that compiles. |
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 %} |
@kostya AFAIK CI complains but please go ahead and open a new PR to try it out. |
Fixes #6405