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

rope.base.ast and rope.base.astutils #582

Merged
merged 8 commits into from Dec 10, 2022
Merged

Conversation

lieryan
Copy link
Member

@lieryan lieryan commented Dec 9, 2022

Important changes:

  • Rename rope.base.{astutils->nameanalyze}
  • Remove usage of standard library ast
  • Make rope.base.ast.parse() have identical interface to stdlib ast.parse()
  • Silence unnecessary flake warnings

Rename rope.base.{astutils->nameanalyze}

astutils is a very bad module name for this, many parts of static analysis can be described as "utilities to work with ast", so it's not really descriptive enough as to what get_name_levels() and _NodeNameCollector really does. The content of this current module isn't actually generic utilities either, they do something very specific, and so calling it "AST utilities" is also very misleading.

As nothing else is actually left in astutils, might as well not have astutils.

Remove usage of standard library ast

rope code should not use standard library ast directly, but only through the rope.base.ast

Make rope.base.ast.parse() have identical interface to stdlib ast.parse()

This function likely can be removed as well, the handling of newlines should really be dealt with at the file access layer, with standard universal newline handling, not here in ast.parse().

Silence unnecessary flake warnings

There are warnings from that aren't really helpful on this module.

…r current code

Background: rope used to use `compile(..., ast.PyCF_ONLY_AST)` instead
of `ast.parse()`
rope code should not use standard library ast directly
`astutils` is a very bad module name for this, many parts of static
analysis can be described as "utilities to work with ast", so it's not
really descriptive enough as to what get_name_levels() and
`_NodeNameCollector` really does. The content of this current module
isn't actually generic utilities either, they do something very
specific, and so calling it `astutils` is also very misleading.
@lieryan lieryan self-assigned this Dec 9, 2022
@lieryan lieryan changed the title Lieryan rope base ast 3 rope.base.ast and rope.base.astutils Dec 9, 2022
@lieryan
Copy link
Member Author

lieryan commented Dec 9, 2022

@edreamleo other than the from ... import * matters (which has been silenced), do you think that this would resolve your other concerns with these two modules have been set up?

We can also rename rope.base.ast to rope.base.rast (rast = Rope's AST) if you still have concerns with the name of the module itself; then we'd be able to just from rope.base import rast anywhere in the codebase, and it would never be ambiguous with stdlib's ast.

@lieryan lieryan merged commit a8fb5bd into master Dec 10, 2022
@lieryan lieryan deleted the lieryan-rope-base-ast-3 branch December 10, 2022 14:56
@lieryan lieryan added this to the 1.6.0 milestone Dec 14, 2022
@edreamleo
Copy link
Contributor

@lieryan Somehow I did not see your question. My apologies for seeming to ignore it.

I don't understand why you say, "rope code should not use standard library ast directly, but only through the rope.base.ast".

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