PR: Again! Disambiguate rope.base.ast and stdlib.ast

[edreamleo]

See Issue #570 for previous discussions.

This PR disambiguates rope.base.ast and stdlib.ast with small, easily understood, diffs.

• Rename rope.base.ast to rope.base.rast.
• Remove from ast import * from rast.py.
• Use rast only to access rast.parse, rast.call_for_nodes, and class rast.RopeNodeVisitor(ast.NodeVisitor)
• Add import ast statements as needed.
• In numpydocstrings.py, add ast qualifier to literal_eval.
• In module_imports, temporarily mark a dubious comment with ###.

Advantages

• The code makes clear from immediate context alone which names come from stdlib.ast
  and which names come from rope.base.rast.
• The diffs show that there is no cost to this new clarity.
• This PR removes misdirections (lies) based on rope.base.ast.
  For example, rope.base.ast.iter_child_nodes actually means ast.iter_child_nodes!
  This mistake was another legacy of from ast import *.
• This PR distinguishes rast.parse from ast.parse.
• This PR makes it easier to wrap stdlib.ast should that ever become desirable.
• Eliminating from ast import * helps all checker tools.

Note: Several of these advantages became clear only after looking at the diffs!

Note: It took longer to write this comment than it did to change the code.

[lieryan]

As discussed before, the "fuzziness" between stdlib's ast and rope.base.ast is intentional. You're not supposed to need to care about the differences. That was somewhat harder when ast.parse() and co have incompatible signatures with stdlib's ast, but this is no longer the case now that the incompatible functions are modified, you can just treat rope.base.ast as stdlib's ast.

If ast and rope.base.ast have to be "disambiguated" as proposed here, that means that you have to not just textually but also mentally distinguish between ast vs rope.base.ast all the time, not just when working on rope.base.ast. From outsider's perspective, it would appear that there's no consistency on when to use ast and when to use rope.base.ast. In the current setup, this is something you would never need to think about, and in the rare cases where you do need to know the difference, you can just use the go-to-definition function of your IDE/editor. And you can summarise the correct way to use the modules in one easy to follow sentence.

If it is important to you that you see the difference, another idea is maybe you can write a syntax highlighter for your editor that would color objects from rope.base.ast and stdlib's ast differently. I'd never have suggested this to most people, but since you're the author of your own text editor, that doesn't seem too far fetched.

Linters often complain about about from ast import * because it is frequently misused in a way that pollutes the module's global namespace. But that's not the situation with rope.base.ast, because we actually do want to import everything here. Creating a shadow module like rope.base.ast is precisely the raison d'etre for from ... import *. PEP8 even addresses this use case specifically:

There is one defensible use case for a wildcard import, which is to republish an internal interface [i.e. stdlib's ast] as part of a public API [i.e. rope.base.ast]

Republishing modules like this is a very common pattern in Python, so there should have been ways to tell tools that you know what you're doing. Even the stdlib's ast module itself does this, you are not supposed to import _ast because that's an internal module that's been re-published by the stdlib's ast, and even though most of ast is actually defined in _ast.

[edreamleo]

@lieryan I don't agree with your comments, but this PR is less important to me than PR #614.

I say this because I don't much care about mypy annotations concerning ast or rast.

[edreamleo]

@lieryan

There is one defensible use case for a wildcard import, which is to republish an internal interface [i.e. stdlib's ast] as part of a public API [i.e. rope.base.ast]
...
Republishing modules like this is a very common pattern in Python

Not all wrappers are a good idea.

you have to not just textually but also mentally distinguish between ast vs rope.base.ast all the time, not just when working on rope.base.ast.

stdlib.ast and rast should be distinguished, and it's dead easy to do so. Any Rope dev knows what stdlib.ast contains! rast contains only a few public names. Distinguishing rast.walk from stdlib.ast.walk takes no extra brain cells. What's the big deal???

maybe you can write a syntax highlighter for your editor that would color objects from rope.base.ast and stdlib's ast differently.

Huh? Why would anyone want to color different python files differently? Leo gives users many options for coloring different languages differently (see below) but that's a different matter entirely.

Leo's wrappers

Leo uses import * in leo.core.leoQt. leoQt hides details of the (substantial!) differences between PyQt5 and PyQt6. This is the only import * in Leo's core.

Leo supports syntax coloring with both QSyntaxHighlighter and pygments. Leo's core doesn't know which highlighter the user has chosen. Leo's leo.core.leoColorizer module masks (wraps) several colorizers.

Converted jEdit description files drive the leo.core.leoColorizer.JEditColorizer class. The JEditColorizer class is a (highly complex) wrapper for QSyntaxHighlighter. No other part of Leo knows anything about this code.

Lastly, Leo can run on multiple Guis, including PyQt5, PyQt6, npyscreen, and flexx. Leo's core doesn't know what Gui is running. Several wrapper/interface modules hide all the details.

Summary

I have lots of experience using wrappers. leoQt uses import * for the purpose sanctioned by PEP8. None of Leo's other modules play games with imports.

Imo, wrapping stdlib.ast is a serious mistake. I can see no benefits whatever. The drawbacks are obvious:

• It interferes with flake8, mypy, and pylint.
• It obscures the differences between stdlib.ast and rast.

[edreamleo]

@lieryan I feel frustrated and thwarted by our lack of agreement on this PR and PR #614.

I would be sad to part company with Rope, but without these PRs I would have no other choice:

• Long-lived branches and related PRs such as PR: (Permanent Draft) record EKR's experiments #600 won't work for me.
• The two import * statements thwart all of python's checkers, especially mypy.
• A throw-away repo would kill all my creative and collaborative juices.

Please accept this PR. I can do no more significant work on Rope without it.

[lieryan]

Leo uses import * in leo.core.leoQt. leoQt hides details of the (substantial!) differences between PyQt5 and PyQt6

That is no different than what rope.base.ast is doing. The original purpose of that module is to hide the differences across different versions of the Python stdlib's ast module, particularly between Python 2.x ast and Python 3.x ast. And during previous discussions, we had just removed the wrappers that have no longer been needed.

But we still need rope.base.ast to hide the differences accross Python 3.7 to Python 3.11, which have substantial differences and I expect will continue to have substantial differences as the Python language evolves.

Currently, part of this version specific details are scattered about in the rope code; some in patchedast, some used to be in the pycompat that we had just removed, some in other places, etc. But rope.base.ast is supposed to be the place to contain those differences.

[lieryan]

I really don't understand what you're trying to get at by "playing games with imports".

It obscures the differences between stdlib.ast and rast

There is supposed to be no differences between stdlib's ast and rope.base.ast; it's backporting functionalities from future ast, and sometimes it's just adapting the ast module to rope's way of doing things, like RopeNodeVisitor.

If we had written visitor classes from scratch, then there shouldn't be a need for RopeNodeVisitor, we could've just used the stdlib's NodeVisitor interface directly; but that wasn't really an option because back when rope first needed a NodeVisitor, it hadn't existed in the stdlib yet, so Rope had to develop its own non-standard interface that ends up not being compatible with what gets finalized in stdlib's NodeVisitor.

[edreamleo]

@lieryan

Leo uses import * in leo.core.leoQt. leoQt hides details of the (substantial!) differences between PyQt5 and PyQt6

That is no different than what rope.base.ast is doing.

I couldn't disagree more. The docstring for leoQt.py is:

"""
General import wrapper for PyQt5 and PyQt6.
Provides the *PyQt6* spellings of Qt modules, classes, enums and constants:
- QtWidgets, not QtGui, for all widget classes.
- QtGui, not QtWidgets, for all other classes in the *PyQt4* QtGui module.
- QtWebKitWidgets, not QtWebKit.
- Enums: KeyboardModifier, not KeyboardModifiers, etc.
"""

The leoQt module is an essential wrapper--it hides irrelevant details. This wrapper allows Leo's core to use Qt6 spellings, even if the user is running Qt5.

Without this wrapper, all clients of leoQt would be "infected" by endless tests on Qt's version. leoQt delegates these messy details to leo.core.leoQt5 and leo.core.leoQt6. Imo, these details must be wrapped.

There is supposed to be no differences between stdlib's ast and rope.base.ast; it's backporting functionalities from future ast, and sometimes it's just adapting the ast module to rope's way of doing things, like RopeNodeVisitor.

rope.base.ast is definitely not an essential wrapper--it is merely a "concept". This PR demonstrates that this concept is unnecessary.

[edreamleo]

@lieryan

If we had written visitor classes from scratch, then there shouldn't be a need for RopeNodeVisitor, we could've just used the stdlib's NodeVisitor interface directly; but that wasn't really an option because back when rope first needed a NodeVisitor, it hadn't existed in the stdlib yet, so Rope had to develop its own non-standard interface that ends up not being compatible with what gets finalized in stdlib's NodeVisitor.

I would be happy to remove RopeNodeVisitor. Let's wait until we agree on this PR before doing that :-)

In my experience, seemingly small cleanups like removing RopeNodeVisitor lead to other cleanups. Sometimes the end result is surprisingly big.

[edreamleo]

@lieryan I'll soon start work on another PR, say PR x, that removes RopeNodeVisitor. This work would be a bit easier if PR x were based on this PR, but these two PRs can be done in either order. Either way we would likely end up without rope.base.ast!

I am going away for the Holidays this Wednesday. I'll be gone for about one week. I might not be able to work on PR x during the Holidays, but removing RopeNodeVisitor will surely be my next project when I return.

In short, I would welcome merging this PR into master, but that's not essential at present.

[edreamleo]

@lieryan Imo, PR #619 replaces this PR for most practical purposes.

[edreamleo]

@lieryan I am going to close this PR in favor of #619. I'll reopen only if you find serious problems with #619.