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 after_<type> callbacks #8889

Merged
merged 1 commit into from Oct 20, 2020
Merged

Add after_<type> callbacks #8889

merged 1 commit into from Oct 20, 2020

Conversation

marcandre
Copy link
Contributor

I double-checked that this has basically no performance impact (less than ~0.1% overall) after #8785 (otherwise that would add ~4-5% overall)

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 15, 2020

Won't after_something be a simpler name? on_after seems a bit confusing to me. :)

@marcandre
Copy link
Contributor Author

Won't after_something be a simpler name? on_after seems a bit confusing to me. :)

I liked that it made it clear what where the callbacks: all methods starting with on_. I can check how more complicated the code is to handle after_.

@marcandre marcandre force-pushed the optimize_callbacks branch 2 times, most recently from 7cb89ac to cccbdca Compare October 16, 2020 03:00
Base automatically changed from optimize_callbacks to master October 16, 2020 03:13
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 17, 2020

Yeah, I got why you named it this way, but the only reason why all callbacks started with on_ is that they are all the same type of callbacks. I also believe it's quite uncommon to combine two prepositions in a name. I'm not that concerned about the naming per se, rather about people understanding what the new callbacks are about.

@marcandre marcandre changed the title Add on_after_<type> callbacks Add after_<type> callbacks Oct 18, 2020
@marcandre
Copy link
Contributor Author

marcandre commented Oct 18, 2020

Changed on_after => after. It is definitely better 👍

@bbatsov May I merge this now, or would you rather wait for 1.0 to be out?

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 20, 2020

@marcandre Merge it! 🚀

I doubled check that this has basically no performance impact (less than ~0.1% overall) after #8882
@mergify mergify bot merged commit def3600 into master Oct 20, 2020
@mergify mergify bot deleted the on_after branch October 20, 2020 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants