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
PR: Fix/suppress all mypy complaints (based on master) #674
Conversation
…essary base-class methods
@lieryan This PR is a milestone in my thinking and workflow:
I plan no further work on this PR. Please let me know if you have any concerns or suggestions. |
@lieryan Please comment on this PR in a timely way. I sometimes feel like I'm talking to myself. If you are too busy to take a close look, just saying that would be courteous. |
@@ -225,6 +228,9 @@ def get_module(self): | |||
current_object = current_object.parent | |||
return current_object | |||
|
|||
def get_name(self): | |||
return None |
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.
IIUC, this method and get_module() aren't supposed to be called, it's just a dummy implementation to satisfy mypy.
Maybe it should raise NotImplementedError or something like that instead to indicate that only the child class versions of these methods should actually get called.
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 true that Rope probably never calls PyDefinedObject.get_name
.
Imo, however, having to create these two methods indicates a more serious flaw in the Abstract classes.
At present, the draft PR #675 implements only step 1 of what might become a multi-step project. This first step merely moves methods from the Abstract
classes into what seem to me to be the more natural places, namely the po.PyFunction
and po.PyClass
classes (where po
is my abbreviation for rope.base.pyobjects
).
Step 2 of PR #675 proposes to make PyDefinedObject
a subclass of po.PyObject
. However, this step is likely controversial and I have no great attachment to this step.
Oh what the heck. I may as well say here something that I was planning to save for a discussion.
After weeks of studying Rope's code I finally see the following:
- Knowledge is a blessing as well as a curse :-)
Knowledge helps experienced devs know what's important and what isn't. - For newbies the lack of knowledge is a curse.
We can drown in details because we don't know which tidbit might be crucial.
That's why your comments about inference were so important to me.
I've just spent several hours revising the second comment of forked issue #3.
I could not have done so without your help. - Imo, Rope uses a strange form of inheritance—inheritance via mro.
I understand what's happening now that I have spent days trying to revise it.
But the class hierarchy will be difficult for newbies to understand without help.
The Theory of Operation should explain in detail what is going on.
The alternative is to use a more straightforward form of inheritance.
The (closed) PR PR: Simplify Rope's class hierarchy #664 demonstrates that such a simplification straightforward!
The PR may have gone too far by eliminating theAbstract
class entirely. It's an open question.
Summary
Newbies will struggle to understand the relations between Rope's most important classes.
PR #664 shows that the class hierarchy can be simplified. If this doesn't happen the Theory of Operation should contain a section about how Rope's classes are related.
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.
Meh. The phrase "inheritance by mro" is a fail.
The Aha is that the classes of interest, po.PyObject
, po.PyDefinedObject
and the three Abstract
classes are (mostly) used as mixin classes. None of these classes are subclasses of the others, except that the Abstract
classes are indeed subclasses of po.PyObject
.
The existing class declarations are:
class BuiltinClass(_BuiltinElement, pyobjects.AbstractClass):
class BuiltinFunction(_BuiltinElement, pyobjects.AbstractFunction):
class BuiltinModule(pyobjects.AbstractModule):
class Generator(pyobjects.AbstractClass):
class Iterator(pyobjects.AbstractClass):
class Lambda(pyobjects.AbstractFunction):
class AbstractClass(PyObject):
class AbstractFunction(PyObject):
class PyObject:
class PyDefinedObject:
class AbstractModule(PyObject):
class PyClass(PyDefinedObject, AbstractClass):
class PyFunction(PyDefinedObject, AbstractFunction):
class _PyModule(PyDefinedObject, AbstractModule):
In other words, PyDefinedObject
acts like a PyObject
even thought PyDefinedObject
is not a subclass of PyObject
. Why? Because all subclasses of PyDefinedObject
are also subclasses of an Abstract
class so that PyObject
appears in the mro.
There is a weird kind of logic in this scheme. It's crazy, but imo it's not crazy enough to be right. PR #664 demonstrates that a simpler (more conventional) scheme is feasible.
@@ -324,15 +324,21 @@ def get_changes( | |||
will be applied to all python files. | |||
|
|||
""" | |||
# To do (in another PR): Create a new var: dest_resource: resource.Reource |
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 wouldn't really recommend creating a new argument here. get_changes() is a public API, pretty much all integrations that supports refactoring would have directly called it. I would be very cautious on changing the API here as it's likely going to break some integrations.
What I'd recommend here is to annotate is_folder()
to become a TypeGuard. Unfortunately, TypeGuard is only available in Python 3.9 and above.
IMO, we don't need to maintain backwards compatibility with type annotation. If static checks doesn't pass in older Python version, as long as the code is unaffected at runtime, that's perfectly fine to me.
Alternatively, we could add development dependency on typing-extensions. which has a backport of typing.TypeGuard
.
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 comment wasn't clear. I wasn't suggesting changing the api, I was suggesting adding a new var (not argument). This change would be an example of what I called (in another reply) "splitting" a var. mypy will complain about vars that hold values of differing types.
Imo, it's no big deal whether the existing code ever changes to please mypy.
@@ -5,7 +5,8 @@ | |||
from rope.base import fscommands | |||
|
|||
try: | |||
from ast import _const_node_type_names | |||
# Suppress the mypy complaint: Module "ast" has no attribute "_const_node_type_names" | |||
from ast import _const_node_type_names # type:ignore |
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 a fan of sprinkling so many type: ignore
everywhere. Is there any way to tell mypy to just ignore untyped modules by default.
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.
@lieryan There are several ways of telling mypy to ignore untyped modules:
- This PR inserts a
type: ignore
as the very first line ofrope.base.prefs
to suppress all checks there. .mypy
config files supposedly can suppress checks to specific modules.
However, this capability seems buggy. It seems to suppress initial checks,
but inferences in other files seem to "bleed into" the supposedly suppressed modules.
One alternative to type: ignore
is annotating something as Any
. However, this just substitutes one suppression pattern for another.
Let's look at the type: ignore
statements in this PR:
- The
type: ignore
on line 8 ofrope.base.ast
is a special case. I don't know any way to remove it. - All the suppressions in
fscommands
,doa
,runmod
,numpydocstrings
, andproject
suppress
warnings about third-party imports that might fail. These suppressions seem harmless and natural. oi.type_hinting_evaluate
: suppress (valid) mypy complaints about the@multi
decorator.
I don't understand why@multi
is needed, so I can't suggest any alternative.
My guess:type: ignore
is likely the only way.- I know of no easy way to avoid the
type: ignore
incontrib.autoimport.sqlite
,
but I didn't try very hard—this file doesn't interest me at present. - Afaik, the
type: ignore
suppressions inrefactor.move
can only be removed by "splitting" a name which is assigned values with different types.
This coding style becomes natural when using mypy. In my experience it always clarifies the code.
But this PR avoids substantive changes to the code.
Summary
Most type: ignore
suppressions seem harmless. The others seem unavoidable.
I'm not bothered by the type: ignore
comments. There aren't that many, and I've carefully documented all the unusual suppressions.
@lieryan Thanks for all your comments. Please resolve the conversations when you are ready. For now I have nothing more to add. I'll be happy to make any changes you would like. I would appreciate getting this PR merged asap. I would like to use it as the base for future work. |
Seems like an unnecessary mental indirection to alias this when it's just used twice in the same type definition. Maybe if there's more of this pair, then it might be worth it to define this alias.
Thanks for fixing these mypy issues |
@lieryan You're welcome! |
@lieryan I wish to acknowledge that my concerns about |
Summary
This PR contains the minimum changes (to the
master
branch) needed to fix and suppress all mypy complaints without removing the wildcard import inrope.base.ast
.This PR:
rope.base.ast
!!# type: ignore
comments.Comments describe all interesting/unusual suppressions.
Details
# type: ignore
as needed to suppress warnings about optional imports.@multi
.The new annotation uses a type alias,
PyObj
, showing that annotations need not be visually daunting.# type: ignore
as the first line of the file.# type: ignore
to compensate for suppressing mypy inrope.base.prefs
.pyobjects.PyObject.get_module
andpyobjects.PyDefinedObject.get_name
.Imo:
Abstract
classes later on.List
andDict
types as needed.Comments suggest actual code revisions, but such changes should wait for a separate PR.