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

Support model.from_json(JSON::Any) #221

Merged
merged 6 commits into from
Jun 8, 2018

Conversation

Blacksmoke16
Copy link
Contributor

@Blacksmoke16 Blacksmoke16 commented Jun 3, 2018

This PR resolves #170 by including support for model.new(JSON::Any)

I just added another layer of ternary that checks of the value is of the JSON::Any type, if it is then use the JSON::Any .as_x to cast it to proper type.

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 3, 2018

Unfortunately this still does not work. might be able to create a wrapper function that will instantiate multiple classes and return an array of them to replicate the Array(Klass).from_json(ARRAY)

class Foo < Granite::Base
  adapter mysql
  table_name foos

  primary type_id : Int64, auto: false
  field capacity : Float32
end
  p Array(Foo).new(JSON.parse(%([{"type_id": 1, "capacity": 69.0},{"type_id": 2, "capacity": 5.0},{"type_id": 3, "capacity": 100.0}])))
Error in src/EveToolsApi.cr:72: no overload matches 'Array(Foo).new' with type JSON::Any
Overloads are:
 - Array(T).new(size : Int, value : T)
 - Array(T).new(ctx : YAML::ParseContext, node : YAML::Nodes::Node)
 - Array(T).new(initial_capacity : Int)
 - Array(T).new(pull : JSON::PullParser)
 - Array(T).new()
 - Array(T).new(ctx : YAML::ParseContext, node : YAML::Nodes::Node, &block)
 - Array(T).new(size : Int, &block)
 - Array(T).new(pull : JSON::PullParser, &block)

  p Array(Foo).new(JSON.parse(%([{"type_id": 1, "capacity": 69.0},{"type_id": 2, "capacity": 5.0},{"type_id": 3, "capacity": 100.0}])))

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 3, 2018

I refactored things to support model.from_json instead of .new to keep inline with standard JSON.mapping usage.

This accounts for creating a single record from a json object, or an array of models from an array of json objects.

The one con of this is that the user has to specify which it is. For example see the spec:

https://github.com/Blacksmoke16/granite-orm/blob/a5108f266e6470c4cafa36425403631de6695863/spec/granite_spec.cr#L70-L97

Notice how in the first i am specifying .as(Review) and in the other i am specifying .as(Array(WebSite)). This isn't the end of the world imo but if anyone has a better idea on how to remove the need for that i would be happy to hear it.

@Blacksmoke16 Blacksmoke16 changed the title Support model.new(JSON::Any) Support model.from_json(JSON::Any) Jun 5, 2018
{% end %}
return
end

return @{{_name.id}} = nil if value.nil?
{% if type.id == Int32.id %}
@{{_name.id}} = value.is_a?(String) ? value.to_i32(strict: false) : value.is_a?(Int64) ? value.to_i32 : value.as(Int32)
@{{_name.id}} = value.is_a?(JSON::Any) ? value.as_i : value.is_a?(String) ? value.to_i32(strict: false) : value.is_a?(Int64) ? value.to_i32 : value.as(Int32)
Copy link
Member

Choose a reason for hiding this comment

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

man, this is some true ugliness on my part. Maybe someday we can figure out how to clean this up.

@drujensen drujensen requested a review from a team June 6, 2018 01:55
Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

Nice addition. The json api isn't very elegant, and I think it's possible to come up with a better DSL than what from_json provides, but I don't think it's necessary for this addition to be helpful.

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 6, 2018

@robacarp it should be clarified that the from_json implemented here is not the same one as Crystals Array(Model).from_json(json)

@robacarp
Copy link
Member

robacarp commented Jun 6, 2018

@Blacksmoke16 yep. Since there are two approvals, are you ready for this to merge?

@Blacksmoke16
Copy link
Contributor Author

I probably should add some info to the README. I'll do that tonight and can merge it tomorrow.

@drujensen
Copy link
Member

@Blacksmoke16 let me know when you are ready.

@Blacksmoke16
Copy link
Contributor Author

Readme is updated. Should be good to go @drujensen

@robacarp robacarp merged commit e5f9b4c into amberframework:master Jun 8, 2018
@robacarp
Copy link
Member

robacarp commented Jun 8, 2018

@Blacksmoke16 thank you!

@Blacksmoke16 Blacksmoke16 deleted the json-any branch July 19, 2018 00:24
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.

Error with normal constructor with JSON.mapping
3 participants