Skip to content

Allows the definition of a customized whodunnit in a small scope#334

Closed
lucasas wants to merge 6 commits intopaper-trail-gem:masterfrom
lucasas:master
Closed

Allows the definition of a customized whodunnit in a small scope#334
lucasas wants to merge 6 commits intopaper-trail-gem:masterfrom
lucasas:master

Conversation

@lucasas
Copy link
Copy Markdown

@lucasas lucasas commented Feb 22, 2014

It's very useful when you want to define who is responsible to change some object and don't considering the global scope or request scope.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this would make more sense instead of having whodunnit and _whodunnit

def whodunnit(whodunnit = PaperTrail.whodunnit)
  @whodunnit = whodunnit
  yield if block_given?
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @seanlinsley, what about the case where I don't want to use my_object.whodunnit?

@batter batter self-assigned this Mar 4, 2014
@batter batter added this to the 3.0.1 milestone Mar 4, 2014
@lucasas
Copy link
Copy Markdown
Author

lucasas commented Mar 10, 2014

Did you have plans to merge this?

@batter
Copy link
Copy Markdown
Collaborator

batter commented Mar 11, 2014

@lucasas - Yes. After reviewing the PR a little more closely just now I'm thinking it probably makes more sense to drop the instance variable store and only allow this method to be called with a block argument to prevent confusion.

As in:

PaperTrail.whodunnit = 'Andy Stewart'

widget = Widget.create!
widget.versions.last.whodunnit # 'Andy Stewart'

# This will work
widget.whodunnit 'Lucas Souza' do
  widget.update_attributes :name => 'Wibble'
end

widget.versions.last.whodunnit # 'Lucas Souza'

# This won't
Widget.whodunnit = 'Lucas Souza' # Throws Error

I just think trying to store the value as an instance variable could create some confusion both with usage and with code readability in terms of adding an additional level of complexity. Does that make sense?

@batter batter closed this in 44a632e Mar 11, 2014
@lucasas
Copy link
Copy Markdown
Author

lucasas commented Mar 12, 2014

Hi @batter, thanks for accept my PR. I think make sense your changes.

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