Skip to content
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

Merged
merged 5 commits into from
Apr 12, 2018

Conversation

Daniel-Worrall
Copy link
Contributor

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.

@@ -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 }}
Copy link
Contributor

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 ?)

Copy link
Contributor Author

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?

Copy link
Member

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.

@@ -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 %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proper indentation please.

it "defines query getter with class restriction" do
{% begin %}
{% methods = JSONWithQueryAttributes.methods %}
{{methods.find{ |m| m.name == "foo?"}.return_type }}.should eq(Bool)
Copy link
Contributor

@Sija Sija Apr 10, 2018

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

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor Author

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.

it "defines query getter with class restriction" do
{% begin %}
{% methods = JSONWithQueryAttributes.methods %}
{{methods.find(&.name.==("foo?")).return_type}}.should eq(Bool)
Copy link
Contributor

@Sija Sija Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the whitespaces, please...

@Daniel-Worrall
Copy link
Contributor Author

This should be good to go assuming we approve the spec being in place

@sdogruyol sdogruyol merged commit bdab495 into crystal-lang:master Apr 12, 2018
@sdogruyol sdogruyol added this to the Next milestone Apr 12, 2018
@Daniel-Worrall Daniel-Worrall deleted the json-type branch April 13, 2018 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants