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 a customize_class_mro plugin hook #4567

Merged
merged 1 commit into from
Sep 3, 2018
Merged

Conversation

snarkmaster
Copy link
Contributor

@snarkmaster snarkmaster commented Feb 13, 2018

The rationale for this MRO hook is documented on #4527

This patch completely addresses my need for customizing the MRO of types that use my metaclass, and I believe it is simple & general enough for other plugin authors.

NB My initial version of this change avoided the "get_*_hook -> Callable" indirection, and simply allowed each plugin to customize the MRO. It seemed like @gvanrossum was not a fan of breaking with the local convention, so I made the API conventional and more complex. Here is the diff from the original version: https://gist.github.com/snarkmaster/85076c9793fd5639fc7212a4c04e710a

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG (nothing changes if there's no MRO hook, and the API looks right now). Can you resolve the merge conflict?

The rationale for this MRO hook is documented on python#4527

This patch completely addresses my need for customizing the MRO of types that use my metaclass, and I believe it is simple & general enough for other plugin authors.

I did `./runtests.py`, and the only failure is in `testCoberturaParser`, which looks completely unrelated to my changes. Log here: https://gist.github.com/snarkmaster/3f78cf04cfacb7abf2a8da9b95298075

PS You will notice in the patch that I deviated from the pattern of "some_hook(fullname) returns a callback, which takes a context". I did this as a suggestion for improvement. At the very least, all of the -> None hooks could be simplified in this fashion (and probably the others, too). I'm happy to put up a patch for that, if there's a process for dealing with a breaking change in the plugin API.

A specific reason I disliked the original pattern is that the provided fullname often does not have enough information for the hook to decide whether it cares, so the hook has to return a callback always. And yet, we spend cycles specifically extracting & passing just the fullname. For this reason, my base class hook is just return base_class_callback.
@snarkmaster
Copy link
Contributor Author

Resolved the merge conflict.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks good to me. @ilevkivskyi OK to merge before the release branch is cut?

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Looks OK to me!

(Just to clarify there are currently no guarantees about plugin API, many things may change when we get more experience with them.)

@ilevkivskyi ilevkivskyi merged commit 940c9b9 into python:master Sep 3, 2018
@snarkmaster
Copy link
Contributor Author

Thanks to all! Acknowledging that there are no guarantees, and that the API may change significantly.

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

Successfully merging this pull request may close these issues.

3 participants