Extract class: Reifier

[jaredbeck]

This refactoring encapsulates the process of reification, separating
it from the model (VersionConcern).

The ABC complexity score of VersionConcern (measured by flog) is
reduced from 750 to 362, a 52% reduction.

Finally, this refactoring provides a truely private namespace for
methods like reify_has_many_through. Such methods will no
longer be mixed into PaperTrail::Version, or end-user's version
models, if any.

[jaredbeck]

Just for fun (it doesn't tell us anything flog can't), here is the Code Climate analysis of this refactoring (attached):

[batter]

Yea, I will agree that the code base as a whole could definitely stand some refactoring into smaller modules and methods of less length, I've certainly allowed some of the methods to become unwieldy over time (not that I can remember it ever being super great in that regard).

Do you think it makes more sense to have this Reifier be a class or a mixin module? Any particular reasons why or why not? And could we potentially memoize the Reifier instance(s) when they are generated if we are going to stick with the class?

[jaredbeck]

Do you think it makes more sense to have this Reifier be a class or a mixin module? Any particular reasons why or why not?

I'd prefer to not mix Reifier into VersionConcern. By not mixing in, we reduce our public API.

.. this refactoring provides a truly private namespace for
methods like reify_has_many_through. Such methods will no
longer be mixed into PaperTrail::Version, or end-user's version
models, if any.

Also, I think this class is easier to understand if it stands alone. It's still tightly coupled to ActiveRecord (a lot of AR methods are called) but they are all called with explicit receiver, so the coupling is more obvious.

And could we potentially memoize the Reifier instance(s) when they are generated if we are going to stick with the class?

I don't think so, not the way it's currently written. Are you concerned about memory usage? I could do some profiling fairly easily.

[jaredbeck]

Mind if I merge this, Ben?

[batter]

Apologies for the delayed review, I've been on vacation. I'll take a closer look this afternoon.

[batter]

What does this comment mean?

[jaredbeck]

@api private indicates a private part of a library's API.

I've seen it, and its inverse @api public, used in major ruby libraries like rspec (https://github.com/rspec/rspec-expectations/blob/14faeab88f319ac0c2e4d793ec02c2b69eb52a5c/lib/rspec/matchers/composable.rb#L9). I think it comes from javadoc originally.

[jaredbeck]

Sorry I've been slow on reviewing this, just started a new job, I'll try to go through it more thoroughly in the next few days, looks good on the surface.

Thanks. Don't rush your review (it's a big change!) but there is going to be a conflict with #596 so it would be great if I didn't have to rebase this too many times. Git conflicts aren't a big deal, but let's not let this get stale if it is something you're interested in. Thanks.

[jaredbeck]

Ben, this is ready for another review. I've changed it from a class to a module as we discussed and I've rebased, resolving the conflict with #596

[jaredbeck]

Mind if I merge this? I'm getting a little antsy because I had to fix another conflict today, sorry.

[batter]

Sorry Jared, was away on a trip with limited access to internet for the last 5 days. This looks good to me, and it is aligned with what we discussed during our conversation. 👍

[jaredbeck]

Sorry Jared, was away on a trip with limited access to internet for the last 5 days.

No worries, thanks for the review, I know it was a big one!