PR 3 for #570: the name 'ast'

[edreamleo]

See #570. Supersedes PR #579.

[edreamleo]

@lieryan Many thanks for your recent comments in the (closed) PR #579.

Instead of sleeping on them, I swam in the lap pool. Standing in the shower and sitting in the locker room I saw:

• What you are trying to do with rope.base.ast.
• What was so confusing to me about rope.base.ast.

We are close to resolving all the issues! We can discuss some minor preferences, but these will not be fraught.

I now understand the code well enough so that I could live with it as it is (except for uncontroversial cleanups). My efforts on these PRs have been a form of initiation. I'm not complaining! I've learned a lot. Btw, I trashed my first fork because I could see no obvious way to get it to a pristine state. Better just to create the new fork.

At last I understand the confusion

In the legacy code, rope.base.ast defines 5 functions:
parse, walk, get_child_nodes, call_for_nodes, and get_children.

Let us assume some module contains the following import:

from rope.base import ast

Experts (like you) and initiated apprentices (like me) "just know" that for x in one of these 5 names, ast.x means rope.base.ast.x. For all other names y, ast.y means stdlib.ast.y!

This problem might be invisible to you because of the "curse of knowledge." I can tell you that this "overloading" of the imported ast symbol was very confusing at first.

What to do

The minimal change would be to leave everything exactly as it is, except for a prominent comment in rope/base/ast.py. This post would be pre-writing for such a comment :-)

I could live with this, but I would like to discuss alternatives. Each has (minor?) drawbacks, but imo each does eliminate the confusion.

Idea 1: Qualify ast.AST nodes with ast.ast.

• In rope.base.ast change from ast import * to import ast.
• Change ast.x to ast.ast.x for all x instdlib.ast.

Idea 2: Import ast from rope.base.ast

• Again, in rope.base.ast change from ast import * to import ast.
• In each module using stdlib.ast, add from rope.base.ast import ast.
• Access stdlib.x via ast.x, exactly as in master.

Other variations on these two themes are possible.

Summary

Doing nothing except adding a comment has some appeal. I can live with it.

Using the ast.ast qualifier might be my first choice.

@lieryan Please let me know your wishes.

[edreamleo]

@lieryan Thanks for the comment. I'll remove all such useless methods.

[edreamleo]

When I awoke this morning, I wanted to push back about rope.base.ast. Sorry, not sorry :-)

I am enjoying our conversation--it is respectful even when heated. We are beginning to know each other as engineers and as persons.

We are making progress, but imo our debate deserves to continue for as long as it takes. Yes, I could live with the code in master, but I would rather not :-)

On to the specifics!

We agree that namespaces are a honking good idea, but significant questions remain about the rope.base.ast module:

• What is its purpose?
• What should it contain?
• How easy is it to understand?
• What is its effect on checking tools?

Purpose: rope.base.ast serves no purpose now. Let's wait until ast2 appears before trying to wrap it!

Contents: There is significant name clash within rope.base.ast. The names parse and walk conflict with ast.parse and ast.walk. The other three functions will probably never need wrapping. Imo, they belong in rope.base.astutils.

Clarity: Devs must understand the internals of rope.base.ast to know the actual meaning of leo.base.ast.x. In most places, ast.x actually does mean stdlib.ast.x. In all those places, rope.base.ast just gets in the way (now). Wrapping stdlib.ast creates a useless indirection.

Tools: from ast import * needlessly hobbles all of our checking tools.

Summary

Not all modules are a honking good idea.

[edreamleo]

@lieryan I'm fixing/suppressing pylint complaints. These two (no, three) complaints look like they point to real problems in the tests:

ropetest\projecttest.py:1014:8: W1503: Redundant use of assertTrue with constant value '.ropeproject' (redundant-unittest-assert)
ropetest\projecttest.py:1066:8: W1503: Redundant use of assertTrue with constant value 'create_file ' (redundant-unittest-assert)
ropetest\projecttest.py:1082:8: W1503: Redundant use of assertTrue with constant value 'create_file ' (redundant-unittest-assert)

The corresponding code:

def test_getting_project_rope_folder(self):
    self.project = testutils.sample_project(ropefolder=".ropeproject")
    self.assertTrue(self.project.ropefolder.exists())
    self.assertTrue(".ropeproject", self.project.ropefolder.path)

def test_normal_fscommands(self):
    fscommands = _MockFSCommands()
    self.project = testutils.sample_project(fscommands=fscommands)
    myfile = self.project.get_file("myfile.txt")
    myfile.create()
    self.assertTrue("create_file ", fscommands.log)

def test_deprecated_fscommands(self):
    fscommands = _DeprecatedFSCommands()
    self.project = testutils.sample_project(fscommands=fscommands)
    myfile = self.project.get_file("myfile.txt")
    myfile.create()
    self.assertTrue("create_file ", fscommands.log)

I'm not sure what was intended. Do you have any suggestions?

[lieryan]

@edreamleo Thanks for finding those issues, these assertTrue seems like they are copy paste errors for what should've been assertEqual()

[edreamleo]

@lieryan I'm good with merging this PR now. It's a big step forward.

Given the size of this PR, and the (routine, I'm guessing) problems merging PR #581, I think it would be good to make any further changes in a new PR.

[edreamleo]

@lieryan I have converted this PR to a draft one again.

I'll incorporate the commits that cleaned imports into a new PR. When that PR becomes part of master I'll merge master into this PR, thereby reducing this PR's size. I'll also study at master to see whether I want to pursue this PR at all.

[edreamleo]

I am going to close this PR. A new PR (if I choose to submit one) will have the effect of squashing commits and rebooting our conversation.

[lieryan]

Locking this thread. Please continue any remaining thread of discussions on #575