Remove modifications to core ruby objects #45
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robotdana
force-pushed
the
dont-override-hashes
branch
from
b4fb251
to
4564258
Compare
robotdana
force-pushed
the
dont-override-hashes
branch
from
4564258
to
b43448b
Compare
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
---
For the same reason as removing Hash#method_missing in the previous
commit. We shouldn't me adding methods to core ruby classes unexpectedly
Especially as there's theoretically more than one way to camel case
things and this method has the very sensible name with no namespace, so
its possible to have a definition of this method with the same name and
slightly different behaviour (e.g. camelCase vs CamelCase,
or camelCaseHTTP vs camelCaseHttp).
WHAT
----
Moved the method from String to Adyen::Service, making it so the gem
only modifies things within the Adyen namespace.
It's a class method so we can get at it from spec_helper too
I also added a spec for all the methods we care about so we can be
confident this change didn't break anything and will continue to work
the same now and in the future.
WHILE I WAS THERE
-----------------
I refactored slightly. it's extremely slightly faster (for now), and I
find it easier to read.
```
STRING = "string_to_camelcase".freeze
require 'benchmark/ips'
Benchmark.ips do |x|
x.report(:theirs) { STRING.split("_").collect.with_index{ |x, i| i > 0 ? x.capitalize : x }.join }
x.report(:mine) { STRING.gsub(/_./) { |x| x[1].upcase } }
x.compare!
end
```
```
Warming up --------------------------------------
theirs 37.069k i/100ms
mine 38.974k i/100ms
Calculating -------------------------------------
theirs 419.977k (± 2.5%) i/s - 2.113M in 5.034341s
mine 448.863k (± 2.0%) i/s - 2.260M in 5.038137s
Comparison:
mine: 448863.3 i/s
theirs: 419976.9 i/s - 1.07x slower
```
(it's about 1.3x slower in 2.3 but slightly faster in 2.4, 2.5, 2.6)
robotdana
force-pushed
the
dont-override-hashes
branch
from
b43448b
to
a74d2d9
Compare
crrood
requested review from
crrood,
Aleffio,
KadoBOT,
rikterbeek and
AlexandrosMor
crrood
approved these changes
Dec 4, 2019
|
Hi @robotdana, Thanks for the PR! We are validating this PR. Regards, |
rikterbeek
approved these changes
Dec 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #44
details in commit messages