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

originator is a bad choice for method name #479

Closed
jasonfb opened this issue Feb 3, 2015 · 4 comments
Closed

originator is a bad choice for method name #479

jasonfb opened this issue Feb 3, 2015 · 4 comments
Milestone

Comments

@jasonfb
Copy link

jasonfb commented Feb 3, 2015

This is really a point of feedback and not a bug.

Many of the papertrail objects that get patched onto a has_paper_trail object are very generic in namespace. I would strongly recommend a namespacing convention (like pt_) for all papertrail methods so that they don't conflict with other method names in the app.

"originator", in particular, makes this gem basically unusable with the Spree ecosystem because many objects in Spree have their own originator method.

I tried a monkey patch using alias_method chain and got a little bit of the way but still it's a bad pattern and I'd recommend changing originator to pt_originator which will surely not cause naming conflicts

@jasonfb jasonfb changed the title originator is a bad choice for object name originator is a bad choice for method name Feb 3, 2015
@batter batter added this to the 4.0.0 milestone Feb 3, 2015
@batter
Copy link
Collaborator

batter commented Mar 14, 2015

Did you also make a suggestion to Spree to namespace their method names?

@jasonfb
Copy link
Author

jasonfb commented Mar 15, 2015

it's a relationship that describes a first-order relationship between two objects. it seems to me that papertrail is more of an add-on and not a first-class system within an app, and using the method 'originator' on all papertrail-tracked objects seems like it surely is asking for collision with anyone else's use of the (relatively generic) term "originator"

It's been removed in later versions of Spree anyway.

@jaredbeck
Copy link
Member

Ben, are you still planning on changing this method name? If so, is this something you'd like help with, or is it too tricky and you want to do it yourself? I'd like to help get a 4.0 release candidate out the door.

@batter
Copy link
Collaborator

batter commented May 6, 2015

No this one is relatively straightforward. It's #518 I'm trying to work on, hopefully will have something by EOD with a fix for the test cases.

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

No branches or pull requests

3 participants