Skip to content

Conversation

@happycollision
Copy link
Contributor

This will keep the validation_errors helper from being less-than-helpful.

Tests the current, desired behavior
Also displays the current, unwanted behavior
It's the least I can do for opening it in the first place.
Copy link
Contributor

@richmolj richmolj left a comment

Choose a reason for hiding this comment

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

Thanks for raising and addressing the issue @happycollision - had one small comment then would love to merge.


def validation_errors
@validation_errors ||= {}.tap do |errors|
return if json['errors'].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change this like to

errors = json['errors'] || []
errors.each do |e|

That way the method always returns a hash, and something like validation_errors[:foo] won't fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The errors variable is already taken as the self of {}.tap. But yes, I have made that change by using a different variable.

I am more familiar with Javascript than Ruby, so I thought returning would still return—wait. I literally just thought of a better way as I was writing my explanation of what I did...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. It is fixed below.

My heart knows Ruby better than Javascript, but my brain knows Javascript better than Ruby.

This extra expectation ensures that `validation_errors[:any_key]` also
won't throw an error.
@happycollision
Copy link
Contributor Author

I did a fixup to the last commit when I realized I had a better solution where we didn't need a new local variable.

@richmolj
Copy link
Contributor

Thanks for the help @happycollision, much appreciated ❤️ ! LGTM 👍

@richmolj richmolj merged commit 6250d87 into jsonapi-suite:master Jan 29, 2018
@richmolj
Copy link
Contributor

Released in 0.4.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants