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

[ci skip] Fix inaccurate callback documentation #42461

Merged
merged 1 commit into from
Jun 13, 2021
Merged

Conversation

Wittiest
Copy link
Contributor

Summary

Fixes #42457

Other Information

The callback de-deduplication logic currently happens at the time callbacks are set on a model. A fix that would implicitly solve a few issues around de-duping would be to apply the following for all callbacks accepting symbolized method names:

  • Rather than front-load de-duplication, have a Set created during the application of each callback (ex: on_update) and dynamically de-duplicate invocations of a specific method here. This would implicitly stop duplicate predicate methods from running in the cases of overlapping if and unless conditions while also allowing for distinct if and unless conditions to pass in the same symbolized method name for the same callback method.

@rails-bot rails-bot bot added the docs label Jun 12, 2021
@ghiculescu
Copy link
Member

I would drop “symbolized” - “method name” should be fine. Do you think it’s worth mentioning that if you use the block form it’s fine to use both?

@Wittiest
Copy link
Contributor Author

@ghiculescu I think it's fine to avoid that detail because people can immediately see that if they want to call the same method, they can just use after_save_commit in the documentation below.

This is more about correcting the documentation which incorrectly makes developers think they cannot use both of these callbacks. The case where devs want to use both of these callbacks instead of after_save_commit would be when they are calling different methods.

@zzak
Copy link
Member

zzak commented Jun 13, 2021

@Wittiest Could you squash your commits please? 🙏

@Wittiest
Copy link
Contributor Author

@zzak Done :)

@zzak zzak merged commit c8f0c64 into rails:main Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate active_record_callbacks documentation
3 participants