Skip to content
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

Closed
wants to merge 8 commits into from
Closed

PR: ekr-grand-reorg: disambiguate class names and simplify qualifiers #614

wants to merge 8 commits into from

Conversation

edreamleo
Copy link
Contributor

@edreamleo edreamleo commented Dec 16, 2022

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:

  • Remove name clashes.
  • Simply imports.

Newly explicit distinctions (disambiguated names)

  • pynames.ParameterNameStub vs. pyobjectsdef.ParameterName
  • pyobjects.AssignedNameStub vs. pyobjectsdef.AssignedName
  • pyobjects.PyComprehensionBase vs pyobjectsdef.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 to pyobjects.

Step 1: Merge pynamesdef and pynames

  1. cff's (global searches) revealed that only the ParameterName name clashed between pynames and pynamesdef.
    Therefore, I renamed (in pynames) ParameterName to ParameterNameStub.
    This is an essential distinction, revealed for the first time in this PR.
    Iirc, I also renamed PyFunction to PyFunctionStub early on.
  2. I then ran unit tests until they passed(!).
    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 and pynamesdef. But without odious imports there is no need to do so!

With the new disambiguated names it was straightforward to restore pynamesdef!

Notes:

  • At no time did I ever encounter circular imports.
  • Even with tiny changes, I could not have kept the required changes straight in my head without Leo.
    However, the notes above and the diffs should be clear enough.

Summary

This PR:

  • Disambiguates all class names, using uniform qualifiers everywhere.
  • Removes the odious import * from pynamesdef. 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.

@edreamleo edreamleo marked this pull request as draft December 16, 2022 17:14
@edreamleo edreamleo marked this pull request as ready for review December 16, 2022 18:01
@edreamleo edreamleo changed the title PR: ekr-grand-reorg: unify pynames/pynamesdef & pyobjects/pyobjectsdef PR: ekr-grand-reorg: unify pynames and pynamesdef Dec 16, 2022
@edreamleo edreamleo marked this pull request as draft December 16, 2022 18:04
@edreamleo edreamleo marked this pull request as ready for review December 16, 2022 22:51
@edreamleo
Copy link
Contributor Author

@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?

@edreamleo edreamleo changed the title PR: ekr-grand-reorg: unify pynames and pynamesdef PR: ekr-grand-reorg: disambiguate class names and simplify qualifiers Dec 16, 2022
@edreamleo edreamleo marked this pull request as draft December 16, 2022 23:01
@edreamleo
Copy link
Contributor Author

@lieryan There is one last nit to resolve before this PR is ready for review.

Just as before, the ImportedModule class resides in pynames, not pynamesdef. However the legacy code misleadingly used the pynamesdef qualifier. The diffs show the new (different, correct) qualifier for ImportedModule.

I don't care where ImportedModule resides. Most of the diffs could be made smaller by moving this class to pynamesdef and restoring the qualifier.

What would you like to do? I'm good either way.

@edreamleo
Copy link
Contributor Author

@lieryan Please merge this PR. Until this happens I will be wasting my time trying to make other contributions to Rope.

@lieryan
Copy link
Member

lieryan commented Dec 19, 2022

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 pyobjects.PyClass but pyobjects.PyFunctionStub, there's no rhyme nor reason why one should be called Stub but the other isn't.

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 pyobjectsdef.DefinedPyClass, pyobjectsdef.DefinedPyFunction, etc. But while that kind of naming convention is accurate, it is also redundant because the containing modules' name (i.e. pyobjectsdef vs pyobjects) already tells you when the code expects an definition or inference objects. The names are already non-ambiguous. The naming of these classes are intended to always be interpreted together with the module names, they aren't supposed to be imported individually.

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: pickle.pickle_loads(), yaml.yaml_loads(), json.json_loads().

@edreamleo
Copy link
Contributor Author

@lieryan Closing this PR in favor of your PR #618.

@edreamleo edreamleo closed this Dec 19, 2022
@edreamleo
Copy link
Contributor Author

@lieryan

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 pyobjects.PyClass but pyobjects.PyFunctionStub, there's no rhyme nor reason why one should be called Stub but the other isn't.

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.

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?

@edreamleo edreamleo deleted the ekr-grand-reorg branch January 1, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants