An idea to combat model pollution

[jaredbeck]

Problem

has_paper_trail adds too many methods to the ActiveRecord model, as described in #703, and others.

If I'm counting correctly, installing the paper_trail gem adds 36 instance
methods and 10 class methods to all of your active record models. Of those
46, 13 have a prefix, either "pt_" or "paper_trail_". I don't know what the
best way to deal with this is. Ideally, we'd add far fewer methods to
people's models. If we were able to add fewer methods to models, then I
wouldn't mind prefixing all of them.
#703

Solution

Add only two methods to the AR model.

1. An instance method #paper_trail
2. A class method .paper_trail

The instance method would return a RecordTrail and the class method would
return a ClassTrail. Those names are totally up for debate.

Advantages

• Plain ruby, easy to understand
• Adding new methods to e.g. the RecordTrail does not add any methods to
  the AR model.
• Method privacy is more strongly enforced.
• Enables isolated testing of e.g. RecordTrail; it can be constructed with a
  mock AR instance.

Disadvantages

• Two new classes, though they are simple.

[seanlinsley]

I think I'd prefer version over paper_trail, as it's easier to type and mirrors the versions association. So you could do:

record.version.live?
record.version.originator
record.version.source
record.version.switched_on?
record.version.save?

[jaredbeck]

I think I'd prefer version over paper_trail, as it's easier to type and mirrors the versions association.

Cool, I'm glad you like the general idea. I'm not opposed to the word version, but I am concerned it's bit of a common word. Any other ideas? "trail" is uncommon and easy to type.

[seanlinsley]

You can get the best of both worlds by making it configurable.

[jaredbeck]

You can get the best of both worlds by making it configurable.

That's true, we could pass the name(s) to has_paper_trail. However, configuration means a more complicated implementation, so if we can choose a name that is likely to satisfy 99% of people, or at least not cause a name conflict for 99% of people, then that would be my first choice.

[batter]

@jaredbeck, apologies for the delay in reviewing this. This seems like a sensible refactor to me, since requests tend to come in pretty consistently to rename method names provided to models that are causing namespace collision in ActiveRecord. The questions is does it make sense for all instance methods (and possibly even class methods) provided by PaperTrail to be 'hidden' / 'extracted' behind a barrier such as this.

I'm not sure what the answer is, but it might be sensible to hold off on this until PaperTrail 5 if the answer is to indeed extract all of them, since it will cause breakage. Or maybe we do the transition in PaperTrail 4 but have the old methods show a deprecation warning, and then remove the old methods once PaperTrail 5 rolls out.

[jaredbeck]

This seems like a sensible refactor to me, since requests tend to come in pretty consistently to rename method names provided to models that are causing namespace collision in ActiveRecord. The questions is does it make sense for all instance methods (and possibly even class methods) provided by PaperTrail to be 'hidden' / 'extracted' behind a barrier such as this.

That's a good question. PT adds at least 36 instance methods by my count. This PR only extracts one of them. Maybe the next step is to try extracting a dozen of them and see if we're still happy with this idea. I'm happy to move slowly on this, as it would be a big change to the public API and I want to get it right.

.. it might be sensible to hold off on this until PaperTrail 5 if the answer is to indeed extract all of them, since it will cause breakage. Or maybe we do the transition in PaperTrail 4 but have the old methods show a deprecation warning, and then remove the old methods once PaperTrail 5 rolls out.

This is a big change, and I don't want it to hold up PT 5. I'm happy if this doesn't happen until PT 6. We could do a 5.99 release that deprecates the old polluting API.

[jaredbeck]

OK, I've taken this from an idea to a proof of concept. I'd like your feedback again, please, to know if I should continue.

Things I like so far:

• No change to our public API.
• Private methods! We'll be able to break up big methods like setup_model_for_paper_trail (currently the largest method in the library) without adding any methods to our public API. ✂️
• The Code Climate GPA has increased by 0.24 so far, partly due to the new private methods. 🎓

Things I don't like:

• The two lambdas defined in ModelConfig are harder to understand. In addition to having the same "execution scope" as before (the model class) they now also have the lexical scope of ModelConfig.
• I had to use send in a few places to call private methods on the model from ModelConfig. For example, attr_accessor.

Next Steps

Sean and Ben, please take another look and let me know your concerns. If we do decide to continue with this, we could then discuss the name of the two important new methods currently named "paper_trail". Thanks for your help!

[jaredbeck]

Status: class methods 63% done, instance methods 28% done. Deprecation scheduled for 5.2, removal in 6.0.

[jaredbeck]

Status: class methods 100% done, instance methods 60% done.

[jaredbeck]

Whew. This is complete, and ready for final review. 7 class methods and 30 instance methods are deprecated, and a handful of private methods are moved without deprecation.

Here are the things I'd like feedback on:

1. Are #paper_trail and .paper_trail the right names for the two most important methods? As Sean pointed out, it's a lot to type. I'm tempted to suggest the more concise #trail, which I think we could get away with. However, #paper_trail is easier to remember.
2. Is ::PaperTrail::RecordTrail a good name? Again, I'm tempted to suggest the simpler ::PaperTrail::Trail.
3. Is ::PaperTrail::ModelConfig a good name? ModelSetup might be better?
4. Is everyone OK with deprecating in 5.2 and dropping in 6.0?

Thanks for the review!

[jaredbeck]

@batter @seanlinsley any feedback? objections?

[seanlinsley]

@jaredbeck sorry, I'll try responding in the next few days.

[jaredbeck]

I'm going to go ahead and merge this. I'll wait another week before releasing it (in 5.2.0) to give y'all one more week to raise any objections. After it's released, it will be much harder to change the names of things, especially the critical .paper_trail method, so speak now or hold your peace :)

[jaredbeck]

Sean and Ben, I'll be releasing this, as 5.2.0, this weekend, unless I hear any objections. If you want another week to review it before it's released, please let me know, thanks.