-
Notifications
You must be signed in to change notification settings - Fork 163
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: ekr-grand-reorg: disambiguate class names and simplify qualifiers #614
Conversation
@lieryan This PR is more successful than I dared dream. It is a grand compromise--I think it gives us both what we want. What do you think? |
@lieryan There is one last nit to resolve before this PR is ready for review. Just as before, the I don't care where What would you like to do? I'm good either way. |
@lieryan Please merge this PR. Until this happens I will be wasting my time trying to make other contributions to Rope. |
Adding a "Stub" suffix here to some objects is conveying totally incorrect and misleading information, it's doing a disservice to misled people reading the code in this way. And it's extremely confusing why is it These aren't stub classes, they are not placeholders, they are actual, real class and there are fundamental conceptual distinction between pynames/pyobjects and pynamesdef/pyobjectsdef. None of these are "just a stub class" the way you have been interpreting them to be. It's not that I don't want to compromise here, but the reasoning you put on is based on completely misunderstanding what these classes does in the codebase. I would hope that you would try to understand what the existing codebase is doing first before making large sweeping changes. If you really have to name things differently, then what you would want to do is rename all the classes with Nobody would complains that pickle.loads, yaml.loads and json.loads have name clashes, because these functions are intended to be imported with the module name, not by themselves. And these libraries wouldn't become better by removing the name clashes by renaming them into: |
This is good stuff. Shouldn't the docstrings for these classes say something along these lines? PR #618 doesn't supply docstrings for any of the classes at issue here. I agree that the 'Stub' suffix is misleading. These classes are the base class for other classes. Do you think a 'Base' prefix would clarify matters? Or some other prefix? |
The PR eliminates empty distinctions and makes the crucial, real distinctions explicit.
Oh joy: there is no need to merge any files! We need only:
Newly explicit distinctions (disambiguated names)
pynames.ParameterNameStub
vs.pyobjectsdef.ParameterName
pyobjects.AssignedNameStub
vs.pyobjectsdef.AssignedName
pyobjects.PyComprehensionBase
vspyobjectsdef.PyComprehension
(PyComprehensionBase is not just a Stub class--it has one method)
pyobjects.PyFunctionStub
vs.pyobjectsdef.PyFunction
pyobjects.PyModuleStub
vs.pyobjectsdef.PyModule
pyobjects.PyPackageStub
vs.pyobjectsdef.PyPackage
For consistency, maybe
ParameterNameStub
should move topyobjects
.Step 1: Merge pynamesdef and pynames
ParameterName
name clashed betweenpynames
andpynamesdef
.Therefore, I renamed (in pynames)
ParameterName
toParameterNameStub
.This is an essential distinction, revealed for the first time in this PR.
Iirc, I also renamed
PyFunction
toPyFunctionStub
early on.I fixed one failure at a time, ensuring that I never got carried away with changes.
I have used this (unintuitive?) tactics many times before, both here and in Leo.
Step 2: The Grand Compromise: undo the merge!
Yes, it is possible to merge
pynames
andpynamesdef
. But without odious imports there is no need to do so!With the new disambiguated names it was straightforward to restore
pynamesdef
!Notes:
However, the notes above and the diffs should be clear enough.
Summary
This PR:
import *
frompynamesdef
. All imports are now straightforward.I am proud of this PR. It took lots of work to make the diffs as small as they are. The diffs clearly confirm the renaming listed above.
At long last Rope's qualifiers are simple, uniform, truthful, and informative.