Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An idea to combat model pollution #719

Merged
merged 1 commit into from
Jun 20, 2016
Merged

Conversation

jaredbeck
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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

@jaredbeck
Copy link
Member Author

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.

@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch from 8b3a04c to aa7b634 Compare March 5, 2016 23:55
@batter
Copy link
Collaborator

batter commented Mar 23, 2016

@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
Copy link
Member Author

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 jaredbeck force-pushed the an_idea_for_model_pollution branch 2 times, most recently from a104b4d to e7ce81a Compare April 10, 2016 07:50
@jaredbeck
Copy link
Member Author

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 jaredbeck added this to the 6.0.0 milestone Apr 19, 2016
@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch 2 times, most recently from 2f36905 to 46d9398 Compare May 20, 2016 05:22
@jaredbeck jaredbeck modified the milestones: 5.2.0, 6.0.0 May 20, 2016
@jaredbeck
Copy link
Member Author

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

@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch 5 times, most recently from 00ee5bd to f7e23e4 Compare May 26, 2016 04:21
@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch 4 times, most recently from 9a7c18c to c8c1233 Compare June 3, 2016 03:24
@jaredbeck
Copy link
Member Author

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

@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch 3 times, most recently from 5859568 to b6545e9 Compare June 5, 2016 20:40
Problem
-------

`has_paper_trail` adds too many methods to the ActiveRecord model.

> 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.
- Methods 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.
@jaredbeck jaredbeck force-pushed the an_idea_for_model_pollution branch from b6545e9 to ad3806f Compare June 6, 2016 05:19
@jaredbeck
Copy link
Member Author

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
Copy link
Member Author

@batter @seanlinsley any feedback? objections?

@seanlinsley
Copy link
Member

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

@jaredbeck
Copy link
Member Author

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 jaredbeck merged commit 26c6ab9 into master Jun 20, 2016
@jaredbeck jaredbeck deleted the an_idea_for_model_pollution branch June 20, 2016 03:23
@jaredbeck
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants