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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix :unknown_attributes assignment for ArrayType #34

Merged

Conversation

@iarie
Copy link
Contributor

@iarie iarie commented Sep 4, 2019

An attempt to resolve this issue: #32

The simple solution is to use ArrayType as wrapper around its @model_klass and delegate type casting to it
I just made a quick look and fix, I'm not sure is there any caveats 馃

Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

Good job on that! We just need to handle a small issue and add entry to the changelog

Loading

end
end

def cast_model_type_value(value)
@model_klass.to_type.cast_value(value)
Copy link
Owner

@DmitryTsepelev DmitryTsepelev Sep 4, 2019

Choose a reason for hiding this comment

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

I think we could memoize @model_klass.to_type to avoid extra object instantiations

Loading

@DmitryTsepelev
Copy link
Owner

@DmitryTsepelev DmitryTsepelev commented Sep 4, 2019

@romansavrulin do you want to review the PR too? 馃檪

Loading

end
end

def cast_model_type_value(value)
@model_klass_type.cast_value(value)
Copy link

@romansavrulin romansavrulin Sep 4, 2019

Choose a reason for hiding this comment

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

Is this enough for handle_unknown_attribute with rescue block on JSON_Type to be called?

def cast_value(value)
case value
when String then decode_and_initialize(value)
when Hash then @model_klass.new(value)
when @model_klass, nil then value
else raise_cast_error(value)
end
rescue ActiveModel::UnknownAttributeError => e
handle_unknown_attribute(value, e)
end

Loading

Copy link
Owner

@DmitryTsepelev DmitryTsepelev Sep 5, 2019

Choose a reason for hiding this comment

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

Yep, having the JsonType instance generated for the current model_class we can call cast_value on it, so it will handle the remaining work for us (rescue the exception and collect unknown attributes)

Loading

@romansavrulin
Copy link

@romansavrulin romansavrulin commented Sep 4, 2019

@DmitryTsepelev Thanks for invitation. As I mentioned before, I'm a kinda junior in ruby and not too familiar with its meta-programming model. But I definitely can try to check this PR in my runtime

Loading

Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

LGTM, merging! Thank you so much

Loading

@DmitryTsepelev DmitryTsepelev merged commit 2cbf55b into DmitryTsepelev:master Sep 5, 2019
2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants