-
Notifications
You must be signed in to change notification settings - Fork 176
PR: Simplify Rope's class hierarchy #664
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
Conversation
|
@lieryan I am going to close this PR. I'll use ekr-fork-rope2 to work on the Theory of Operation without bothering you. |
|
@lieryan I am going to reopen this PR as a draft. This PR is one of three approaches I am considering: PR #664 (This PR):Replace PRs #664 and #677 both work. PR #677 has zero impact on performance. The other two PRs likely have negligible impact on performance, but more testing is needed. |
This PR collapses the complexity of several type-inference classes:
Abstractclasses with one new arg toPyObject.__init__.PyObjectorPyDefinedObject.PyDefinedObjectis a subclass ofPyObject.The annotation,
Union[PyObject, PyDefinedObject], can be replaced byPyObject.Note: The
ekr-mypy-single-inheritancebranch (PR #665) annotates this branch, with the usual changes torope.base.astneeded to let mypy do complete checking. Those checks found three missing functions that have now been moved from twoAbstractclasses into the appropriate classes inpyobjects.py.Summary of the changes, in the order they occurred:
get_base_typeandget_unknownfunctions inpyobjects.py.Abstractclasses.AbstractClass,AbstractFunction, andAbstractModuleclasses.Step 1: Simplify the
get_base_typeandget_unknownfunctionsget_base_typeandget_unknownfunctions inpyobjects.py.PyObject._get_base_typestatic method.Step 2: Remove most methods from the
AbstractclassesAbstractclasses, retaining 3 methods found nowhere else.get_superclassesfrom theBuiltinObjectto its base class,BuiltinClass.With this change,
BuiltinObjecthas the same form as the threeAbstractclasses.Step 3: Remove all three Abstract classes
rope/base/utils/predicates.py.isinstance(obj, Abstract*)withis_abstract_*(obj).Note: Neither the
get_base_typenor theget_unknownfunction requires theAbstractclasses.Abstractclasses entirely.Discussion:
Abstractclasses as markers.The legacy pattern is
if isinstance(obj, AbstractX):The new pattern is
if is_abstract_x(obj):. This pattern is shorter, but is less explicit.Abstractclasses help init theirPyObjectbase classes, but Part 4 shows that this is "faux help".The formerly "abstract" classes need only add one new arg when instantiating
PyObject.Step 4: Use single inheritance for all type-inference classes
This step was unexpectedly very easy.
PyDefinedObjectnow hasPyObjectas its only base class.The
PyObjectannotation can replaceUnion[PyObject, PyDefinedObject].PyDefinedObjectas their only base class:po.PyClass,po.PyComprehension,po.PyFunction,po._PyModule.pod.PyFunction,pod.PyClass,pod.PyComprehension