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

Already on GitHub? Sign in to your account

Prevent callback from being set twice. #7376

Merged
merged 1 commit into from Dec 21, 2012

Conversation

Projects
None yet
4 participants
Contributor

dmitriy-kiriyenko commented Aug 17, 2012

When you add one callack in two separate set_callback calls - it is only called once.

When you do it in one set_callback call - it is called twice.

This violates the principle of least astonishment for me. Duplicating callback is usually an error. There is a correct and obvious way to do anything without this "feature".

If you want to do:

before_save :clear_balance, :calculate_tax, :clear_balance

or whatever, you should better do:

before_save :carefully_calculate_tax

def carefully_calculate_tax
  clear_balance
  calculate_tax
  clear_balance
end

And this even opens gates for some advanced refactorings, unlike the first approach.

My assumptions are:

  • Principle of least astonishment is violated, when callbacks are either prevented from duplication, or not.
  • Duplicating callbacks is usually an error. When it is intentional - it's a smell of a bad design and can be approached without abusing this "feature".

My suggestion is: do not allow duplicating callbacks in one callback call, like it is not allowed in separate callbacks call.

P.S. It's currently one test on railties failing in master and so in this branch also. It's not me!

 1) Failure:
test_0006_reload constants on development(LoadingTest) [test/application/loading_test.rb:135]:
Expected: "2"
  Actual: "1"
Member

steveklabnik commented Nov 17, 2012

This will need a rebase.

@jonleighton @tenderlove what do you think about this feature?

Contributor

dmitriy-kiriyenko commented Nov 28, 2012

I've updated the branch with rails/master. Tests pass now.

Can you add a changelog entry as well, explaining the issue?

Contributor

dmitriy-kiriyenko commented Nov 28, 2012

Done. Is that ok, or should I be more descriptive?

Looks fine I guess, although an example of the issue could be good.

/cc @josevalim @rafaelfranca

Contributor

dmitriy-kiriyenko commented Nov 28, 2012

Done.

Contributor

dmitriy-kiriyenko commented Dec 14, 2012

Can you answer something?

Owner

rafaelfranca commented Dec 14, 2012

This needs a rebase, But seems fine.

Contributor

dmitriy-kiriyenko commented Dec 14, 2012

Done.

Contributor

dmitriy-kiriyenko commented Dec 15, 2012

Come on! Is it fine, or it is not, or you are going in 2 weeks again say it needs a rebase?

@dmitriy-kiriyenko please be patient, we have lots of issues to review and give feedback. I'll review yours again later if anyone else does before me, it's in my todo list. Thanks for your work on that.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Dec 20, 2012

activesupport/test/callbacks_test.rb
@@ -607,6 +607,45 @@ def save
end
end
+ class OneThoThreeSave
@carlosantoniodasilva

carlosantoniodasilva Dec 20, 2012

Owner

Should this be OneTwoThreeSave?

Prevent callback from being set twice.
When you add one callack in two separate `set_callback` calls - it is
only called once.

When you do it in one `set_callback` call - it is called twice.

This violates the principle of least astonishment for me. Duplicating
callback is usually an error. There is a correct and obvious way to do
anything without this "feature".

If you want to do

    before_save :clear_balance, :calculate_tax, :clear_balance

or whatever, you should better do

    before_save :carefully_calculate_tax

    def carefully_calculate_tax
      clear_balance
      calculate_tax
      clear_balance
    end

And this even opens gates for some advanced refactorings, unlike the
first approach.

My assumptions are:

- Principle of least astonishment is violated, when callbacks are either
  prevented from duplication, or not.
- Duplicating callbacks is usually an error. When it is intentional -
  it's a smell of a bad design and can be approached without abusing
  this "feature".

My suggestion is: do not allow duplicating callbacks in one callback
call, like it is not allowed in separate callbacks call.
Contributor

dmitriy-kiriyenko commented Dec 20, 2012

Done.

rafaelfranca added a commit that referenced this pull request Dec 21, 2012

Merge pull request #7376 from dmitriy-kiriyenko/fix-double-callback-i…
…n-same-statement

Prevent callback from being set twice.

Conflicts:
	activesupport/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit 4a9644a into rails:master Dec 21, 2012

Owner

rafaelfranca commented Dec 21, 2012

Thanks and sorry for the delay. As @carlosantoniodasilva said, we have a lot of things to do and sometimes things can take a little longer than expected.

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