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.
? should come directly after the type (Bool? not Bool ?)
There was a problem hiding this comment.
This is exactly as it is in the setter above. Should both be changed?
There was a problem hiding this comment.
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 %} | |||
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.
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.
This code does not work.
There was a problem hiding this comment.
@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.
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.
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.
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.