-
Notifications
You must be signed in to change notification settings - Fork 13
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
Attempt to make the test suite pass under mypy 1.0.0 #89
Conversation
To avoid having to update all the samples
To stop mypy from complaining that they have empty bodies
Implicit optionals are no longer accepted by mypy 1.0
Since python/mypy#13729, mypy complains if a non-abstract function has a trivial body. Avoid this providing a dummy body.
# Thus we can make (1) and (2) true by setting abstract_status to some value | ||
# distinct from these three NOT_ABSTRACT, IS_ABSTRACT and IMPLICITLY_ABSTRACT. | ||
# These are presently the integers 0, 1, and 2 defined in mypy/nodes.py:738-743. | ||
func_def.abstract_status = HACK_IS_ABSTRACT_NON_PROPAGATING |
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 is very unorthodox, but if it will let us to upgrade to mypy-1.0, I'm all for it! :)
src/mypy_zope/plugin.py
Outdated
@@ -479,6 +482,9 @@ def _analyze_zope_interface( | |||
replacement = self._adjust_interface_function(api, cls.info, item) | |||
elif isinstance(item, OverloadedFuncDef): | |||
replacement = self._adjust_interface_overload(api, cls.info, item) | |||
elif isinstance(item, Decorator): | |||
replacement = item | |||
replacement.func = self._adjust_interface_function(api, cls.info, item.func) |
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 seems to be causing error in the description. What this change is designed for?
The following patch to your branch makes all tests pass:
diff --git a/src/mypy_zope/plugin.py b/src/mypy_zope/plugin.py
index e9aeca7..02ab413 100644
--- a/src/mypy_zope/plugin.py
+++ b/src/mypy_zope/plugin.py
@@ -482,9 +482,6 @@ class ZopeInterfacePlugin(Plugin):
replacement = self._adjust_interface_function(api, cls.info, item)
elif isinstance(item, OverloadedFuncDef):
replacement = self._adjust_interface_overload(api, cls.info, item)
- elif isinstance(item, Decorator):
- replacement = item
- replacement.func = self._adjust_interface_function(api, cls.info, item.func)
else:
continue
diff --git a/tests/samples/interface_meta.py b/tests/samples/interface_meta.py
index 2e04617..94d63fa 100644
--- a/tests/samples/interface_meta.py
+++ b/tests/samples/interface_meta.py
@@ -7,7 +7,7 @@ T = TypeVar('T', bound='zope.interface.Interface')
class IBookmark(zope.interface.Interface):
@staticmethod
def create(url: str) -> 'IBookmark':
- pass
+ return IBookmark(url)
def createOb(iface: Type[T]) -> T:
if zope.interface.interfaces.IInterface.providedBy(iface):
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.
Well, actually my patch is stupid, interface cannot have an implementation, disregard it...
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.
That create
method didn't make too much sense anyways. I suggest this, unless there's something important about handling decorators:
diff --git a/src/mypy_zope/plugin.py b/src/mypy_zope/plugin.py
index e9aeca7..02ab413 100644
--- a/src/mypy_zope/plugin.py
+++ b/src/mypy_zope/plugin.py
@@ -482,9 +482,6 @@ class ZopeInterfacePlugin(Plugin):
replacement = self._adjust_interface_function(api, cls.info, item)
elif isinstance(item, OverloadedFuncDef):
replacement = self._adjust_interface_overload(api, cls.info, item)
- elif isinstance(item, Decorator):
- replacement = item
- replacement.func = self._adjust_interface_function(api, cls.info, item.func)
else:
continue
diff --git a/tests/samples/interface_meta.py b/tests/samples/interface_meta.py
index 2e04617..35a7d42 100644
--- a/tests/samples/interface_meta.py
+++ b/tests/samples/interface_meta.py
@@ -5,8 +5,6 @@ T = TypeVar('T', bound='zope.interface.Interface')
class IBookmark(zope.interface.Interface):
- @staticmethod
- def create(url: str) -> 'IBookmark':
pass
def createOb(iface: Type[T]) -> T:
@@ -20,6 +18,6 @@ def main(self) -> None:
"""
<output>
-interface_meta.py:19: note: Revealed type is "__main__.IBookmark"
+interface_meta.py:17: note: Revealed type is "__main__.IBookmark"
</output>
"""
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.
What this change is designed for?
The IBookmark
example that you cite. I thought the staticmethod was intended to be part of the IBookmark
interface, so that classes which implement it would have to define their own create
method.
If that's not the case then let's apply your patch!
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 don't even think interfaces can define static methods... At least it doesn't make sense to me today.
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 suggest this, unless there's something important about handling decorators:
Ahh I see, the test doesn't even make use of create
. Let's try the second patch.
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 looks good to me. Take it out of WIP whenever you're ready to merge this.
And thanks for digging into it!
I've just done so. (I wanted to run this PR + mypy 1.0 against https://github.com/matrix-org/synapse/. There weren't any new errors related to the plugin AFACIS, other than those covered by #88.)
My pleasure. I just hope the hack doesn't come back to bite us in the future! |
I'm sure it will eventually, but we'll deal with it when it happens :) |
Released in mypy-zope-0.9.0 |
For your consideration: an attempt to update this plugin to work with the recent mypy 1.0.0 release.
#85 was an attempt to establish compatibility with mypy 0.991. It was partially successful, but ultimately blocked on python/mypy#14106
I wanted to take a look to see if could make any progress. I think I was able to workaround the linked mypy issue with a dirty, brittle and naughty hack (see da1d0fb). It might be too brittle to include upstream; let me know what you think.
On my machine, all but one test passes. I am not sure how to handle the failure:
Fixes #82 (I think?)
Closes #85