MONGOID-5702: Monkey Patch Removal: Remove __numeric__ class method from Numeric module

[johnnyshields]

It is now inlined into its only usage. It was previously declared @api private so assume it is safe to delete.

[johnnyshields]

This should be a straightforward merge.

[jamis]

@johnnyshields thanks for this one, yeah, it should be totally safe to apply, thanks to that private flag.

However, I feel like this is a step backward in readability. The evolve method is harder to grok at a glance. I understand wanting to remove monkeypatched methods on standard classes (three cheers for fixing that, hurray!) but I feel like there's still value in isolating the responsibility that __numeric__ currently holds.

Mongoid::Utils might be a reasonable host for such functionality, if the goal is primarily to remove that monkeypatch.

[johnnyshields]

this is a step backward in readability

@jamis how so? The previous method was:

 def evolve(object) __evolve__(object) do |obj| __numeric__(obj) rescue obj # <-- this line is now inlined end end

There is nothing particularly "readable" about the old method--by reading this, can anyone really tell me what __evolve__ vs. __numeric__ does? I doubt it...

Mongoid::Utils might be a reasonable host for such functionality

This PR is a stepping stone to putting evolve and/or __evolve__ in a Util, so there's no need to consider to put __numeric__ in a Util...

[jamis]

@johnnyshields we'll have to agree to disagree, then. I find the original version infinitely more readable -- the intention is immediately apparent by glancing at the implementation. By inlining the implementation of __numeric__, we've "unrefactored" it, taking a step backward (IMO) and requiring a bit more mental work to parse what the intent of the method is.

I won't try to convince you, though; it's fine if you have a different opinion. But I'm not comfortable inlining the __numeric__ method in evolve, there.

[johnnyshields]

Readability is neither here nor there-- the goal of this PR is to get rid of monkey patches. It's far better to "unrefactor" to remove the monkey patches first, then the "re-refactor" them into a sustainable, non-monkey patched structure. If we don't take this approach, and puritanically insist that all methods be maximally readable at all times, we'll never actually get rid of our monkeys 🐒 🐒 🐒

Put another way, trading a slightly less-readable method for eliminating a monkey patch is a good trade--and is a two-way door.

[jamis]

@johnnyshields -- we aren't going to agree on this. Ultimately, we do agree that the monkey-patches on core classes should be minimized. I suggested moving __numeric__ to the Utils module as a step toward that end. I could say that you responded rather puritanically yourself to that idea, yeah? 😜

Regardless, inlining __numeric__ is off the table (unless there were significant performance gains to be had from it, or some similar argument).