In response to omniauth/omniauth#773 (comment)
Responses to your comments in turn.
I'd feel worse about omniaith with this pull request than without.
Agreed, that's why we closed it. This PR didn't go far enough. It tried to support the current interface, in its (almost) entirety when the right way to do it would be to introduce proper config with real honest to goodness objects and methods. I talked about this omniauth/omniauth#773 (comment)
I feel that it's taking on a chunk of technical debt.
The current config object being based on Hashie has TREMENDOUS technical debt, which is why this PR was so difficult and why the OmniStruct class can't be any smaller. This was my core argument in the article. Again, as i said before we shouldn't replicate all of Hashie's features, we should define an actual interface and use it.
Said this earlier "we might as well use custom objects instead of struct or hash types"
I bet this PR has more code without Hashie than with Hashie.
Nope. This PR has 112 lines added and 32 lines deleted (net of 80 lines added). Removing the Hashie dependency (only mash) is around 213 lines (without comments). Hashie depends on Hashie::Hash which has another 56 lines pretty inspect at 19 lines and stringify keys at another 50 lines. For a grand total of (213+56+19+50) 338 lines. This makes the change (80 - 338) NEGATIVE 258 lines. 💥
Now i know what you're thinking, hashie is a dependency, we don't get to count those lines. Well...when you use a third party library you become a maintainer of that code, Michael actually wrote the first version of Hashie and was quite literally the maintainer of the code. Even if that wasn't the case, if one of those 338 lines breaks something in omniauth, it's the maintainer of omniauth's job to find it, report it or fix it. It also increases the surface area that the maintainer needs to know.
Let's take a look. If you're using Hashie::Mash you need to know a bunch of methods:
Hashie::Mash.instance_methods.count
# => 176
And it doesn't include all those magic methods added by method_missing.
OmniAuth::OmniStruct.instance_methods.count
# => 71
In comparison, my proposed spike has less than half the methods (but the same method missing behavior). By introducing Hashie into your project you need to know more.
So we've got fewer lines of code to maintain and a much more defined interface (176 - 71 = 105 fewer methods).
I encourage people to use and share libraries but you're kidding yourself if you don't think you introduce technical debt into your project every dependency you add. When Rails 2 to Rails 3 broke a ton of gems and simultaneously a bunch of gem authors decided they didn't want to maintain those gems anymore. Guess what...every single dependency in your app became your responsibility. If it's code that is running on your server, it doesn't matter how it got there, it's your responsibility.
Time to implement a solution is even another! [measure of code]
I completely agree here. When Omniauth passes your Rails app a config object and you accidentally call a method on it that doesn't exist and no exceptions get raised there is a very real cost to the time you will spend fixing your Rails code when an explicit interface would have made the problem go away. If i could accurately measure the times i've misconfigured an omniauth strategy and spent hours debugging, I would gladly list them here.
I think what you're getting at, is that you don't want to maintain things like hashes with indifferent access and deep_merge! methods. I think you shouldn't have to. A proper configuration object isn't based off of a hash, so it doesn't have to worry about symbol or string access and or any merge problems. By needing these types of behaviors for configuration, it's a red flag...i shouldn't have had to write that code.
hashie's business is not omniauth's business.
I disagree. Omniauth must know about how hashie works to perform correctly. When you touch code that hashie touches, you must know how to use it. Omniauth uses hashie, therefore you must know how hashie works to work on omniauth.
Omniauth's needs only a subset of hashie behavior, but any detail that it pulls from hashie should be managed with extreme caution.
The problem here is we pass this config object that is based on Hashie to 3rd party libraries and "strategies", even if it only needs a subset of the behavior, it is giving all the behavior to all the libraries. So while this code doesn't call a specific method, a third party library could. If we were going to actually go forwards, we would need to deprecate like crazy see my comment and example code here: zmoazeni/harvested#83 (comment)
I see someone who is focused on a tiny bit of performance and personal values, with no mention of the costs.
The performance gains here are anything but tiny. Five percent of total rails application request speed isn't tiny, especially considering that Omniauth shouldn't be doing anything on those requests. I struggle for months on code in Rails internals to get just 1~2% faster on an overall request. 5% gain on one relatively small code change is HUGE. While i emphasized performance in the pull request, i should have talked about the biggest benefits or rather the "costs".
The "costs" here provide us with a cleaner interface, less total lines of code to maintain and fewer methods to have to understand due to removing Hashie as a dependency.
Choices in programming are usually a trade-off. You get something, but you give something away. A reasoned approach to changes involved acknowledging both sides. I raise a red flag when I see the person making the changes focused on one.
What i think you meant here was "You seem to be only focussing on the performance, how does this impact usability? Anything else this gives up?" The point of the article is that removing hashie makes your code faster, cleaner, and easier to use. You're giving up flexibility, but it's a false flexibility. The flexibility provided by Hashie also introduces problems.
However, when the refactor takes the form of a large class with its own responsibilities, I think it becomes necessary to test it in isolation. In the form that OmniStruct is in currently, I would not trust that it is fully covered. In a class with revision, I'd consider this unsafe.
Totally agree. What you might not realize is that it took me 16 hours to write this pull request and get all the tests working. I did it on my flight to and from RubyKaigi in Japan. This doesn't include all the time I spent writing the debugging and benchmarking tools. That was just to get the existing tests to pass. After nearly two straight (business) days of coding, i'm more interested in throwing the idea over the fence and seeing what comes of it than shooting for 100% on rcov. I can assure you that had anything merged into master I would have tested the crap out of it.
Telling a closed pull request it needs tests is not helpful.
that performance boost is not free.
You're right it's not free. It took me 16 hours to write that patch, and as we've mentioned it's not even the final solution. If hashie had never been introduced in the first place, then it would have been like getting a 5% performance boost for from the start. Did i mention we would also get a better interface and easier code to work with? Removing Hashie is a win, win, win. Because when I win and you win, we all win.
Note github doesn't send notifications on comments to gists