Fix JSON::Serializable with 2 args, and YAML::Serializable with 3 arg… #6458
Conversation
| @@ -35,6 +35,16 @@ class JSONAttrPerson | |||
| end | |||
| end | |||
|
|
|||
| struct JSONAttrPersonWithTwoFieldInInitialize | |||
There was a problem hiding this comment.
Add a test for single field too.
There was a problem hiding this comment.
there is many classes with 1 arg already
|
Please restore the |
|
i actually have some another problems with my code (inheritance and includes), after this commit, trying to understand why |
This reverts commit 86bf9bd.
|
@kostya any progress? |
|
Bug in my project not related to this commits, it happens on crystal master also, but hard to find why. works in 0.25.1. So i think this can be merged. instantiating 'super(JSON::PullParser)' |
|
bisect to ee6ec50, here problem with @json_unmapped appears. |
src/yaml/serialization.cr
Outdated
| @@ -138,7 +138,9 @@ module YAML | |||
| end | |||
| end | |||
|
|
|||
| def initialize(ctx : YAML::ParseContext, node : ::YAML::Nodes::Node, dummy : Nil) | |||
| def initialize(*, context_for_yaml_serializable : YAML::ParseContext, node_for_yaml_serializable : ::YAML::Nodes::Node) | |||
| ctx = context_for_yaml_serializable | |||
There was a problem hiding this comment.
You could avoid the local assignations by using external/internal naming of the parameters:
def foo(*, external_long_name name : String, external_long_age age : Int32)
{name, age}
end
foo(external_long_name: "John", external_long_age: 30) # => {"John", 30}There was a problem hiding this comment.
What? never see this before
src/json/serialization.cr
Outdated
| @@ -117,7 +117,7 @@ module JSON | |||
|
|
|||
| def self.new(pull : ::JSON::PullParser) | |||
| instance = allocate | |||
| instance.initialize(pull, nil) | |||
| instance.initialize(pull_for_json_serializable: pull) | |||
There was a problem hiding this comment.
Might I recommend the usage of double underscore to name the parameters? Macro variables get converted into __temp_1, perhaps we should use something similar to reference the private-yet-public interface?
There was a problem hiding this comment.
IMO that's too much. There's nothing like that used in stdlib ATM.
There was a problem hiding this comment.
Another option then will be to make initialize private, which can then only be called by this particular .new constructor 🤷♂️ , thus discouraging externals call to it.
|
@bcardiff Maybe this would be nice to have for 0.26.1 |
…s, fixed #6405