Hi. Please don't define Hash#method_missing about adyen-ruby-api-library HOT 7 CLOSED

I don't expect to use NoMethodErrors, they're a way to tell me i've done something wrong.

This changes the contract of every single ruby hash in a codebase. it might respond to a method, or not, there's no way of knowing because it depends on the data, also respond_to_missing? wasn't defined so respond_to? is no use.

This affect on all hashes doesn't seem to be documented anywhere, and is unavoidable if you're using this gem for the straightforward reason of interacting with the adyen api.

If one wanted this 'all hashes to have accessors' behaviour in their codebase they should directly use a gem that adds this to hash, for example the http://github.com/hashie/hashie gem (which is extremely configurable, and won't do more than you want it to), and not as a side effect of trying to interact with a payment gateway, which I would never expect to modify anything about hashes at all.

If the good feedback you've got is just adyen api responses to have accessors for every item of data deeply then the adyen api responses should use a different class than Hash that perhaps has your method_missing (and adds respond_to_missing?) or OpenStruct or something.

--

Another potential issue with defining Hash#method_missing for this is that anything else that adds methods to hashes would win over accessing data, whether it's new ruby versions or other gems. This is probably not something desired.

--

A third reason to do it is you have to play nice with other code. you don't call super from your Hash#method_missing, instead directly raising, so any mixin that's supposed to be calling method_missing can't do its own thing.

--

As a general rule, unless a gem is explicitly and documentedly about adding methods to core ruby classes, it shouldn't add methods to core ruby classes. (including your to_camel_case, though that isn't as egregious)

from adyen-ruby-api-library.

Hi Dana,

Thank you for your input. Can you please elaborate on the use case for relying on Hash NoMethodError's? We've gotten good feedback on this functionality from other users, so I'd like to understand your side of the argument better before making a decision.

Best,
Colin

from adyen-ruby-api-library.

Hi Dana,

Well said, it's a valid point that we may have overstepped our bounds by changing behavior of the core Hash class, and that such changes should be left to dedicated libraries.

Are you willing to submit a pull request to the /lib/adyen/util.rb file with one of your suggestions to change the implementation of AdyenResponse so it doesn't overwrite Hash while maintaining the dot-notation functionality?

Best,
Colin

from adyen-ruby-api-library.

done #45

from adyen-ruby-api-library.

HI Dana,

Thank you for this, it's a very thorough PR - I see you removed the to_camel_case override as well.

After a first read-through I don't see any issues, but given its size I'd like to sync with the team internally before merging.

I'll keep you posted.

Best,
Colin

from adyen-ruby-api-library.

Hi Dana,

Just wanted to let you know we haven't forgotten about this, but are still in the process of reviewing. We'll keep you posted.

Best,
Colin

from adyen-ruby-api-library.

Hi Dana,

Thanks for your patience - we've passed this around the team and all agree it looks great.

I'll merge and release now.

Best,
Colin

from adyen-ruby-api-library.