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

Allow deep indifferent access on Hanami::Utils::Attributes #137

Merged
merged 1 commit into from
May 2, 2016

Conversation

Erol
Copy link
Contributor

@Erol Erol commented Apr 28, 2016

Address hanami/controller#70 by making Hanami::Utils::Attribute#get return Hanami::Utils::Attributes for hash-like attribute values.

@coveralls
Copy link

coveralls commented Apr 28, 2016

Coverage Status

Coverage increased (+0.0005%) to 98.203% when pulling f59b51a on Erol:allow-deep-indifferent-access into b5a7903 on hanami:master.

@@ -51,6 +51,14 @@ def to_h
attributes.get('23').must_equal 'foo'
end

it 'allows deep indifferent access' do
attributes = Hanami::Utils::Attributes.new(foo: {baz: 'bar'})
attributes.get(:foo).get(:baz).must_equal 'bar'
Copy link
Member

Choose a reason for hiding this comment

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

good one, can we test against the nested string syntax instead? eg: params.get('address.street')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I forgot to test that one. New commit coming up.

@runlevel5 runlevel5 added this to the v0.8.0 milestone Apr 29, 2016
@Erol
Copy link
Contributor Author

Erol commented Apr 29, 2016

How about this?

@coveralls
Copy link

coveralls commented Apr 29, 2016

Coverage Status

Coverage increased (+0.002%) to 98.204% when pulling 3ba875c on Erol:allow-deep-indifferent-access into b5a7903 on hanami:master.

@coveralls
Copy link

coveralls commented Apr 29, 2016

Coverage Status

Coverage increased (+0.002%) to 98.204% when pulling 99f6325 on Erol:allow-deep-indifferent-access into b5a7903 on hanami:master.

@runlevel5
Copy link
Member

looks good, the test fails for other unrelated reason. Would you be able to have a look at that too?

break unless value

value = value[key]
end
Copy link
Member

Choose a reason for hiding this comment

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

What do you think, maybe is better to get only last key from split instead unnecessary enumeration? I. e.:

value = @attributes

key = attribute.to_s.split(?.).last
value = value[key] if value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wouldn't be able to cover deep access with that, like for example:

attributes = Hanami::Utils::Attributes.new({address: {street: 'Street', city: 'City'}})
attributes.get("address.street")

Copy link
Member

Choose a reason for hiding this comment

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

yep, you're right!
Thanks for explanation! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Erol
Copy link
Contributor Author

Erol commented May 1, 2016

@joneslee85: Can't reproduce the error on my local, but sure I'll also take a look at it.

@Erol Erol force-pushed the allow-deep-indifferent-access branch from 99f6325 to 2bc5ce3 Compare May 2, 2016 02:16
@coveralls
Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage increased (+0.002%) to 98.204% when pulling 2bc5ce3 on Erol:allow-deep-indifferent-access into b5a7903 on hanami:master.

@Erol
Copy link
Contributor Author

Erol commented May 2, 2016

I squashed my commits and Travis already passed. Maybe a brittle test for file permissions?

@runlevel5 runlevel5 merged commit 9488f24 into hanami:master May 2, 2016
@Erol Erol deleted the allow-deep-indifferent-access branch May 2, 2016 06:47
@Erol
Copy link
Contributor Author

Erol commented May 2, 2016

Thanks for the merge!

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.

None yet

4 participants