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

AS::Callbacks#run_callbacks optimized to reduce backtrace #5999

Merged
merged 1 commit into from Apr 29, 2012

Conversation

Projects
None yet
3 participants
@bogdan
Contributor

bogdan commented Apr 26, 2012

I remember @tenderlove was saying once that reduce backtrace size is a good idea.
And maybe it is especially good for Callbacks because they might appear several times per action call and also they are so ugly.

So, this patch reduce it by one

  activerecord (3.2.3) lib/active_record/connection_adapters/abstract/connection_pool.rb:467:in `call'
  actionpack (3.2.3) lib/action_dispatch/middleware/callbacks.rb:28:in `block in call'
  activesupport (3.2.3) lib/active_support/callbacks.rb:405:in `_run__3245509492598542750__call__654369043753597113__callbacks'
-  activesupport (3.2.3) lib/active_support/callbacks.rb:405:in `__run_callback'
  activesupport (3.2.3) lib/active_support/callbacks.rb:81:in `run_callbacks'
  actionpack (3.2.3) lib/action_dispatch/middleware/callbacks.rb:27:in `call'
  actionpack (3.2.3) lib/action_dispatch/middleware/reloader.rb:65:in `call'
# This generated method plays caching role.
#
def __run_callbacks(kind, object, &blk) #:nodoc:

This comment has been minimized.

@jeremy

jeremy Apr 26, 2012

Member

Should probably keep the __run_callbacks method for compatibility, even if we happen to bypass it.

@jeremy

jeremy Apr 26, 2012

Member

Should probably keep the __run_callbacks method for compatibility, even if we happen to bypass it.

This comment has been minimized.

@bogdan

bogdan Apr 27, 2012

Contributor

Don't think it's a good idea.
__run_callbacks API already changed since Rails 3.2:
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/callbacks.rb#L396

So there is no compatibility already.
Renaming is good idea because it should reflect more the thing it does.

@bogdan

bogdan Apr 27, 2012

Contributor

Don't think it's a good idea.
__run_callbacks API already changed since Rails 3.2:
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/callbacks.rb#L396

So there is no compatibility already.
Renaming is good idea because it should reflect more the thing it does.

@jeremy

View changes

Show outdated Hide outdated activesupport/lib/active_support/callbacks.rb
@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 29, 2012

Contributor

I ran the tests, and this is good to go against Master

cc @jeremy

Contributor

jeremyf commented Apr 29, 2012

I ran the tests, and this is good to go against Master

cc @jeremy

jeremy added a commit that referenced this pull request Apr 29, 2012

Merge pull request #5999 from bogdan/callbacks
AS::Callbacks#run_callbacks optimized to reduce backtrace

@jeremy jeremy merged commit a89827e into rails:master Apr 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment