- Notifications
You must be signed in to change notification settings - Fork941
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Multiple fixes and enhancements for monkey pluggability#1449
base:master
Are you sure you want to change the base?
Conversation
1. `_check_repatching` now merges kwargs into state2. `_get_patch_all_state` allows to retrieve the patch_all configuration state3.a `_patch_module` now allows to specific `_package_prefix` parameter that defaults to 'gevent.'3.b `import_module` is now used in `_patch_module` to allow handling of package names of any depth.4. `patch_item` and both `patch_modules` now accept argument `_patch_module` for item's `__module__` override.fixesgevent#1447
Thanks for this PR! I apologize for the delayed response, I've been fully occupied with other projects. I'll get back to this as soon as I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks again for this PR. I'm very interested in developing a patching API that's truly usable and useful for external plugins.
gevent master is in a good state again after the SSL updates broke things, so nowon Travis we can see one test failure that seems to be related to these changes. I also asked for some clarification and discussion on some of the changes.
olditem = getattr(module, attr, _NONE) | ||
if olditem is not _NONE: | ||
saved.setdefault(module.__name__, {}).setdefault(attr, olditem) | ||
setattr(module, attr, newitem) | ||
if _patch_module: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this is a good idea in general. One example that springs to mind is pickling; another is that it may tend to produce very confusingrepr
results. Why would we want to do this?
If this is something that's for very specialized use-cases, perhaps that would be better served in the hook functions for the module? (See below for some general comments about underscore-prefixed things.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reviewing#1447 I see that pickling was specifically mentioned as the motivation for this. I'm still not convinced, I'd need to see a specific example to understand what problem this solves that can't be solved by the object itself.
@@ -119,6 +119,7 @@ def patch_psycopg(event): | |||
from __future__ import absolute_import | |||
from __future__ import print_function | |||
import sys | |||
from importlib import import_module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was concerned about this extra import either leading to more imports that might cause issue with monkey-patching, or with it taking time for cases that we might not need it.
But on Python 3, it's imported by default anyway during startup, so those are completely moot.
On Python 2, it's a trivial file with no imports besidessys
.
_call_hooks=True): | ||
_call_hooks=True, | ||
_patch_module=False, | ||
_package_prefix='gevent.'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the tradition of 'explicit is better than implicit', it would probably be better to make this a required argument instead of defaulting.
Instead of an extra argument, what ifname
was required to be the fully-qualified name instead of the simple module name? That would allow patches that consist only of a module, not a package. (With a small amount of work, that could generalize to allowing arbitrary objects to be the patch source.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).
gevent_module =getattr(__import__('gevent.' + name), name) | ||
gevent_module =import_module(_package_prefix + name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably better namedsource_module
at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).
@@ -951,9 +964,15 @@ def patch_signal(): | |||
_patch_module("signal") | |||
def _get_patch_all_state(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This function doesn't seem to be used here.
Here's my general philosophy about the underscore prefixed functions and arguments in this module: They're an implementation detail, subject to change. (That's obvious.) But we also know that, as external plugins get going, until we feel ready to publish some sort of official monkey-patching API, they'll be used outside the gevent package. If functionality that gevent itself doesn't use is only used by those external plugins, its much more likely that that functionality is going to break in a way that is not caught by the gevent tests, or be refactored away without planning too. So it's best to keep the implementation details to a minimum and be sure that gevent has a use-case for them all.
In this case, it looks like it would be easy to make_check_repatching
use this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This function is supposed to be used externally by plugins. I was hesitant to make it public, but then again maybe I should've.
I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).
def _check_repatching(**module_settings): | ||
_warnings = [] | ||
key = '_gevent_saved_patch_all' | ||
module_settings.update(module_settings['kwargs']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not clear to me what difference this makes. Could you clarify please? It may be missing a test case, or perhaps that's the reason for the broken test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. Silly me. I just reviewed#1447 and see where the problem was mentioned. So this either is the reason for the test failure, or it definitely needs a specific test.
56b1951
to352434b
Comparebae7acd
tod29d50a
Compare
_check_repatching
now merges kwargs into state_get_patch_all_state
allows to retrieve the patch_all configuration state3.a
_patch_module
now allows to specific_package_prefix
parameter that defaults to 'gevent.'3.b
import_module
is now used in_patch_module
to allow handling of package names of any depth.patch_item
and bothpatch_modules
now accept argument_patch_module
for item's__module__
override.fixes#1447