-
Notifications
You must be signed in to change notification settings - Fork 42
Use metaclass for constructing RemoteInterface #76
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
Changes from all commits
be717ad
44ce706
4100bdf
ff3b9e8
d70d8ec
a57be96
682def4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
|
|
||
| import six | ||
| import types | ||
| import inspect | ||
| from zope.interface import interface, providedBy, implementer | ||
|
|
@@ -28,10 +29,9 @@ class RemoteInterfaceClass(interface.InterfaceClass): | |
|
|
||
| """ | ||
|
|
||
| def __init__(self, iname, bases=(), attrs=None, __module__=None): | ||
| def __init__(self, iname, bases=(), attrs=None): | ||
| if attrs is None: | ||
| interface.InterfaceClass.__init__(self, iname, bases, attrs, | ||
| __module__) | ||
| interface.InterfaceClass.__init__(self, iname, bases, attrs) | ||
| return | ||
|
|
||
| # parse (and remove) the attributes that make this a RemoteInterface | ||
|
|
@@ -41,8 +41,7 @@ def __init__(self, iname, bases=(), attrs=None, __module__=None): | |
| raise | ||
|
|
||
| # now let the normal InterfaceClass do its thing | ||
| interface.InterfaceClass.__init__(self, iname, bases, attrs, | ||
| __module__) | ||
| interface.InterfaceClass.__init__(self, iname, bases, attrs) | ||
|
|
||
| # now add all the remote methods that InterfaceClass would have | ||
| # complained about. This is really gross, and it really makes me | ||
|
|
@@ -92,10 +91,6 @@ def _parseRemoteInterface(self, iname, attrs): | |
|
|
||
| return remote_name, remote_attrs | ||
|
|
||
| RemoteInterface = RemoteInterfaceClass("RemoteInterface", | ||
| __module__="pb.flavors") | ||
|
|
||
|
|
||
|
|
||
| def getRemoteInterface(obj): | ||
| """Get the (one) RemoteInterface supported by the object, or None.""" | ||
|
|
@@ -412,3 +407,10 @@ def _makeConstraint(t): | |
| return LocalInterfaceConstraint(t) | ||
|
|
||
| addToConstraintTypeMap(interface.InterfaceClass, _makeConstraint) | ||
|
|
||
|
|
||
| # See | ||
| # https://github.com/warner/foolscap/pull/76/commits/ff3b9e8c1e4fa13701273a2143ba80b1e58f47cf#r549428977 | ||
| # for more background on the use of add_metaclass here. | ||
| class RemoteInterface(six.with_metaclass(RemoteInterfaceClass, interface.Interface)): | ||
| pass | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good to me, as far as I understand it. I don't really understand the difference between If you have some insight into why one or the other of these methods is preferable, putting it in a comment here might be useful in the long run.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I honestly don't fully understand the nuances, and I was the seminal author for I attempted to use diff --git a/src/foolscap/remoteinterface.py b/src/foolscap/remoteinterface.py
index 04ffd06..d8ef365 100644
--- a/src/foolscap/remoteinterface.py
+++ b/src/foolscap/remoteinterface.py
@@ -409,5 +409,6 @@ def _makeConstraint(t):
addToConstraintTypeMap(interface.InterfaceClass, _makeConstraint)
-class RemoteInterface(six.with_metaclass(RemoteInterfaceClass, interface.Interface)):
+@six.add_metaclass(RemoteInterfaceClass)
+class RemoteInterface(interface.Interface):
passDetailsThe way I read that error message, the Interface class already was instrumented in such a way that the intermediate class that gets created when using Unfortunately, the history behind the addition of My instinct here is that this code works well enough and attempting the alternate approach causes the tests to fail, so there's not a whole lot more to say. I'll add a comment linking to this discussion for those interested in more background. (682def4) |
||
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.
The motivation for this change, the change to MANIFEST.in, and the addition of the empty py.typed file is not clear from the ticket or the PR description.
Maybe these changes belong with some other issue?
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.
The motivation is indicated in d70d8ec. The core of the change is adding a
py.typedfile to the package, which is what communicates to mypy that this package is typed and thus it (and its dependencies) may be used for static type checking. To see the failures on Tahoe, make this change:And then run the typechecks. Here are the errors: