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

Add No Touching #12772

Merged
merged 1 commit into from Nov 13, 2013
Merged

Add No Touching #12772

merged 1 commit into from Nov 13, 2013

Conversation

@dmathieu
Copy link
Contributor

@dmathieu dmathieu commented Nov 5, 2013

This adds #no_touching on all ActiveRecord models, allowing to ignore touching for the duration of a block.

Example:

ActiveRecord::Base.no_touching do
  Post.first.touch #=> Does nothing
end

Comment.no_touching do
  Comment.first.touch #=> Does nothing
  Post.first.touch #=> Updates, but won't update related comments if there are any
end

cc @dhh

end
end

class << self
Copy link
Member

@rafaelfranca rafaelfranca Nov 5, 2013

Choose a reason for hiding this comment

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

Why is not this inside the ClassMethods module?

Loading

Copy link
Contributor Author

@dmathieu dmathieu Nov 5, 2013

Choose a reason for hiding this comment

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

If I put them in the ClassMethods module, I can't access them through NoTouching.apply_to, like we do at line 22.
And it means the model gets the apply_to and applied_to? methods, while here, they're only in the module.

Loading

Copy link
Member

@rafaelfranca rafaelfranca Nov 5, 2013

Choose a reason for hiding this comment

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

Oh, right. Please put # :nodoc: in the methods

Loading

Copy link
Contributor Author

@dmathieu dmathieu Nov 5, 2013

Choose a reason for hiding this comment

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

Done! 🌞

Loading

@egilburg
Copy link
Contributor

@egilburg egilburg commented Nov 5, 2013

I think without_touch or 'without_touching` sounds better.

Loading

@dhh
Copy link
Member

@dhh dhh commented Nov 5, 2013

I like the fun of no_touching.

On Nov 5, 2013, at 8:33, Eugene Gilburg notifications@github.com wrote:

I think without_touch or 'without_touching` sounds better.


Reply to this email directly or view it on GitHub.

Loading

@sausman
Copy link

@sausman sausman commented Nov 5, 2013

I like the sound of no_touch.

Loading

@nijikon
Copy link

@nijikon nijikon commented Nov 5, 2013

+1 for no_touch

Loading

@mbhnyc
Copy link
Contributor

@mbhnyc mbhnyc commented Nov 6, 2013

no_touchy_touchy maximizes fun, while also maximizing awkwardness!

Loading

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Nov 6, 2013

From experience the "fun" things in rails don't stick around (I'm thinking of the snowman for example).
So I renamed it to no_touch in my pull request.

Loading

@nijikon
Copy link

@nijikon nijikon commented Nov 6, 2013

👍

Loading

@dhh
Copy link
Member

@dhh dhh commented Nov 6, 2013

Eh, what? Look up #forty_two. Do rename it back.

On Nov 6, 2013, at 16:59, Damien Mathieu notifications@github.com wrote:

From experience the "fun" things in rails don't stick around (I'm thinking of the snowman for example).
So I renamed it to no_touch in my pull request.


Reply to this email directly or view it on GitHub.

Loading


private
def klasses
@klasses ||= []
Copy link
Member

@rafaelfranca rafaelfranca Nov 6, 2013

Choose a reason for hiding this comment

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

This code is not thread safe. We need to store in a thread local variable.

Loading

Copy link
Contributor Author

@dmathieu dmathieu Nov 12, 2013

Choose a reason for hiding this comment

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

I've fixed the thread safe problem.

Loading

@dhh
Copy link
Member

@dhh dhh commented Nov 12, 2013

Besides these few comments I just added, are we otherwise ready to merge?

Loading

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Nov 13, 2013

I just applied your two comments. :shipit:

Loading

rafaelfranca added a commit that referenced this issue Nov 13, 2013
@rafaelfranca rafaelfranca merged commit 3d2e8cb into rails:master Nov 13, 2013
1 check failed
Loading
sikachu
Copy link
Member

sikachu commented on b32ba36 Nov 13, 2013

Choose a reason for hiding this comment

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

Loading

dmathieu
Copy link
Contributor

dmathieu commented on b32ba36 Nov 13, 2013

Choose a reason for hiding this comment

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

💚

Loading

tomdale
Copy link

tomdale commented on b32ba36 Nov 13, 2013

Choose a reason for hiding this comment

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

http://gifrific.com/wp-content/uploads/2013/02/No-Touching-George-Bluth-Arrested-Development.gif

Loading

fphilipe
Copy link
Contributor

fphilipe commented on b32ba36 Nov 14, 2013

Choose a reason for hiding this comment

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

@dmathieu Could you give an example of a use case for this? I guess it could come in handy, but I can't come up with an example from the top of my head. 😊

Loading

dmathieu
Copy link
Contributor

dmathieu commented on b32ba36 Nov 14, 2013

Choose a reason for hiding this comment

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

@fphilipe: this was a feature request coming from @dhh. I don't have a personal use case for it, I just did the implementation.
He says, as a use case:

Useful when copying object graphs while preserving the original timestamps, for example.

Loading

fphilipe
Copy link
Contributor

fphilipe commented on b32ba36 Nov 14, 2013

Choose a reason for hiding this comment

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

@dmathieu OK, thanks!

Loading

dhh
Copy link
Member

dhh commented on b32ba36 Nov 14, 2013

Choose a reason for hiding this comment

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

Here are two use cases from Basecamp:

def create_taggings_from_mass_assignment
  if @tag_names
    # Normally, tagging a taggable touches the taggable to expire
    # its caches. We can skip the touches here since we're in an
    # after_create callback and we know there are no caches to
    # expire. (Skipping these touches also lets us preserve the
    # original timestamps when copying messages and uploads with
    # tagged attachments.)

    ActiveRecord::Base.no_touching do
      set_tag_names(@tag_names)
    end

    @tag_names = nil
  end
end
class Copier
  def initialize(source, account)
    @source, @account = source, account
  end

  def copy
    ActiveRecord::Base.no_touching do
      @creator = find_person || create_person
      @project = create_project

      copy_todolists
      copy_messages
      copy_documents

      @project.reset_counters
      @project
    end
  rescue
    @project.try(:destroy)
    raise
  ensure
    @creator.try(:trash)
  end

Loading

@dmathieu dmathieu deleted the no_touching branch Nov 13, 2013
@lukaszx0
Copy link
Member

@lukaszx0 lukaszx0 commented Nov 21, 2013

I liked @steveklabnik's idea of calling it #cant_touch_this the most 🤘

Loading

@guilleiguaran
Copy link
Member

@guilleiguaran guilleiguaran commented Nov 21, 2013

LOL :trollface:

Sent from my iPhone

On 20 Nov 2013, at 09:02 pm, Łukasz Strzałkowski notifications@github.com wrote:

I liked @steveklabnik's idea of calling it #cant_touch_this the most


Reply to this email directly or view it on GitHub.

Loading

@arenoir
Copy link

@arenoir arenoir commented Apr 18, 2014

@dmathieu this is very handy, however it seems after_touch callbacks still get run. Is there a way to disable after_touch callbacks?

Loading

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

Successfully merging this pull request may close these issues.

None yet