Hi. Please don't define Hash#method_missing #44
Comments
|
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, |
|
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 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) |
|
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, |
WHY === Previously, monkey-patching Hash 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. Another potential issue with defining Hash#method_missing for this was that anything else that adds methods to hashes would win over accessing data, whether a new ruby versions or other gems. This was probably not something desired. A third reason not to do it is you have to carefully play nice with other code. This Hash#method_missing wasn't call super, instead directly raising, so any mixin that's supposed to be calling method_missing can't do its own thing. --- This affect on all hashes doesn't seem to be documented anywhere, and was unavoidable if you were using this gem for the straightforward reason of interacting with the adyen api. If one wants this 'all hashes to have accessors' behaviour in their codebase they should directly use a gem that adds this to hash, for example the https://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. 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. How === JSON.parse conveniently lets us chose a different class for objects than the default hash Hash, so we can simply use our own class, one that inherits from Hash and define the method_missing there. While i was making changes I added `ArgumentError` for when arguments are passed unexpectedly, and `respond_to_missing?` so `respond_to?` and `method`/`instance_method` work as expected, and super, so anyone intentionally defining `Hash#method_missing` can still see their desired behaviour. There are also specs for this behaviour now. (because when i deleted the Hash#method_missing all the specs still passed). fixes: Adyen#44
WHY === Previously, monkey-patching Hash 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. Another potential issue with defining Hash#method_missing for this was that anything else that adds methods to hashes would win over accessing data, whether a new ruby versions or other gems. This was probably not something desired. A third reason not to do it is you have to carefully play nice with other code. This Hash#method_missing wasn't call super, instead directly raising, so any mixin that's supposed to be calling method_missing can't do its own thing. --- This affect on all hashes doesn't seem to be documented anywhere, and was unavoidable if you were using this gem for the straightforward reason of interacting with the adyen api. If one wants this 'all hashes to have accessors' behaviour in their codebase they should directly use a gem that adds this to hash, for example the https://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. 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. How === JSON.parse conveniently lets us chose a different class for objects than the default hash Hash, so we can simply use our own class, one that inherits from Hash and define the method_missing there. While i was making changes I added `ArgumentError` for when arguments are passed unexpectedly, and `respond_to_missing?` so `respond_to?` and `method`/`instance_method` work as expected, and super, so anyone intentionally defining `Hash#method_missing` can still see their desired behaviour. There are also specs for this behaviour now. (because when i deleted the Hash#method_missing all the specs still passed). fixes: Adyen#44
|
done #45 |
WHY === Previously, monkey-patching Hash 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. Another potential issue with defining Hash#method_missing for this was that anything else that adds methods to hashes would win over accessing data, whether a new ruby versions or other gems. This was probably not something desired. A third reason not to do it is you have to carefully play nice with other code. This Hash#method_missing wasn't call super, instead directly raising, so any mixin that's supposed to be calling method_missing can't do its own thing. --- This affect on all hashes doesn't seem to be documented anywhere, and was unavoidable if you were using this gem for the straightforward reason of interacting with the adyen api. If one wants this 'all hashes to have accessors' behaviour in their codebase they should directly use a gem that adds this to hash, for example the https://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. 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. How === JSON.parse conveniently lets us chose a different class for objects than the default hash Hash, so we can simply use our own class, one that inherits from Hash and define the method_missing there. While i was making changes I added `ArgumentError` for when arguments are passed unexpectedly, and `respond_to_missing?` so `respond_to?` and `method`/`instance_method` work as expected, and super, so anyone intentionally defining `Hash#method_missing` can still see their desired behaviour. There are also specs for this behaviour now. (because when i deleted the Hash#method_missing all the specs still passed). fixes: Adyen#44
|
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, |
|
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, |
|
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, |
NoMethodError is a wonderful thing to have, and subverting that by responding to literally every possible method depending on the hash keys is a potential source of issues.
Please either use [] and .fetch depending if you want it to raise, or if you must, use ruby stdlib ostruct instead of hashes, or if you really really must, use refinements, so that your Hash#method_missing doesn't affect every codebase using your gem.
Thank you
I really wasn't expecting this to happen.
The text was updated successfully, but these errors were encountered: