-
-
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
Fix JSON::Serializable with 2 args, and YAML::Serializable with 3 arg… #6458
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for single field too.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO that's too much. There's nothing like that used in stdlib ATM.
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.
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