Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
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

Open
arcivanov wants to merge1 commit intogevent:master
base:master
Choose a base branch
Loading
fromarcivanov:issue_1447

Conversation

arcivanov
Copy link
Contributor

  1. _check_repatching now merges kwargs into state
  2. _get_patch_all_state allows to retrieve the patch_all configuration state
    3.a_patch_module now allows to specific_package_prefix parameter that defaults to 'gevent.'
    3.bimport_module is now used in_patch_module to allow handling of package names of any depth.
  3. patch_item and bothpatch_modules now accept argument_patch_module for item's__module__ override.

fixes#1447

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
@jamadden
Copy link
Member

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.

Copy link
Member

@jamaddenjamadden left a 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:
Copy link
Member

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.)

Copy link
Member

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
Copy link
Member

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.'):
Copy link
Member

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.)

Copy link
ContributorAuthor

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)
Copy link
Member

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.

Copy link
ContributorAuthor

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():
Copy link
Member

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.

Copy link
ContributorAuthor

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'])
Copy link
Member

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.

Copy link
Member

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.

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

@jamaddenjamaddenjamadden left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Monkey pluggability enhancements and bugs
2 participants
@arcivanov@jamadden

[8]ページ先頭

©2009-2025 Movatter.jp