-
-
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
Include type restriction on getters in JSON/mapping #5935
Conversation
src/json/mapping.cr
Outdated
@@ -80,7 +80,7 @@ module JSON | |||
{% end %} | |||
|
|||
{% if value[:getter] == nil ? true : value[:getter] %} | |||
def {{key.id}} | |||
def {{key.id}} : {{value[:type]}} {{ (value[:nilable] ? "?" : "").id }} |
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.
?
should come directly after the type (Bool?
not Bool ?
)
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.
This is exactly as it is in the setter above. Should both be changed?
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.
It parses with whitespace, but it would be better to remove it.
spec/std/json/mapping_spec.cr
Outdated
@@ -507,6 +507,14 @@ describe "JSON mapping" do | |||
json.bar?.should be_false | |||
end | |||
|
|||
it "defines query getter with class restriction" do | |||
{% begin %} | |||
{% methods = JSONWithQueryAttributes.methods %} |
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.
proper indentation please.
spec/std/json/mapping_spec.cr
Outdated
it "defines query getter with class restriction" do | ||
{% begin %} | ||
{% methods = JSONWithQueryAttributes.methods %} | ||
{{methods.find{ |m| m.name == "foo?"}.return_type }}.should eq(Bool) |
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 can use shorthand &.
notation. Also, please, use spaces in a way they improve readability:
{{ methods.find(&.name==("foo?")).return_type }}.should eq(Bool)
ditto below
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.
This code does not work.
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.
@Daniel-Worrall sorry, I've made a typo (missing .
), this works:
{{ methods.find(&.name.==("foo?")).return_type }}.should eq(Bool)
@@ -507,6 +507,14 @@ describe "JSON mapping" do | |||
json.bar?.should be_false | |||
end | |||
|
|||
it "defines query getter with class restriction" do |
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.
I'm not sure it makes sense to have a spec for this if there is no real reflection. The return type is optional anyway.
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.
It being optional doesn't mean we shouldn't be testing the optional case of supplying it.
spec/std/json/mapping_spec.cr
Outdated
it "defines query getter with class restriction" do | ||
{% begin %} | ||
{% methods = JSONWithQueryAttributes.methods %} | ||
{{methods.find(&.name.==("foo?")).return_type}}.should eq(Bool) |
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 the whitespaces, please...
This should be good to go assuming we approve the spec being in place |
Addresses #5920
Was unsure of a better way to spec this but I'm very aware of how dirty it is and how it relies on macro code more than anything else.