Conversation
lib/siren_client/action.rb
Outdated
| raise ArgumentError, "You must pass in a Hash to SirenClient::Action.new" | ||
| end | ||
| @payload = data | ||
| @payload = data.deep_symbolize_keys |
There was a problem hiding this comment.
Did a quick search and I couldn't find this method anywhere (other than rails). It doesn't look like it's in the ruby stdlib.
There was a problem hiding this comment.
O, okay. It's coming from the ActiveSupport dependency.
| expect(client).to be_a SirenClient::Entity | ||
| end | ||
| it 'to access properties' do | ||
| expect(client.properties['name']).to eq('Test 1') |
There was a problem hiding this comment.
Unfortunately we can't change the spec files (besides adding to them) or we risk breaking backwards incompatibility. I know some usages of this library that depend on the string key when accessing things like properties. It would be great to access the keys both ways but wasn't a requirement of the ticket. (I'm merely saying this so you don't feel obligated to rewrite this, I think you could revert the spec files and be alright)
There was a problem hiding this comment.
Ok, good to know. I'll revert that. There's another activesupport method, with_indifferent_access, that would allow the use case you're describing. That also should allow for reverting changes to the setters like @payload[:name] = xxx back to @payload['name'], while still permitting users to access properties either by payload[:name] or payload['name] or payload.name. Probably should have just reached for with_indifferent_access to begin with. Thoughts @cha55son?
There was a problem hiding this comment.
We'd probably want to go with a more optimized solution (and may be worth pushing to another ticket) as it seems ActiveSupport::HashWithIndifferentAccess has poor performance compared to standard ruby hash lookups. https://gist.github.com/tiagoamaro/c82a27aceedfc901b081
Leaving the output keys as is and allowing input keys of strings/symbols should be sufficient. Regarding the specs, I was wrong. We do want to keep your changes where you create objects with symbols, but we also want to keep the string versions as well.
There was a problem hiding this comment.
@cha55son After further reflection I think it makes the most sense to use deep_stringify_keys to handle any symbolized keys. To more easily test both scenarios where the object is initialized with a hash of stringified or symbolized keys, I've extracted the specs for the relevant SirenClient classes into shared examples. In addition to testing the default values, the tests are now also checking that the values are being set as expected. Let me know what you think! Thanks.
bdfinlayson
force-pushed
the
feature/issue-16
branch
from
8bc14c4 to
b7809a9
Compare
3 participants
This PR introduces support for creating SirenClient instances with objects containing either stringified or symbolized keys. Please see #16 for more information.