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

Make MaybeTree the main Python AST entrypoint for constructing the syntax tree #3550

Merged
merged 22 commits into from
Jan 28, 2025

Conversation

JCZuurmond
Copy link
Member

@JCZuurmond JCZuurmond commented Jan 20, 2025

Changes

Make MaybeTree the main Python AST entrypoint for constructing the syntax tree:

  • Move the @classmethods and @staticmethods that construct a MaybeTree from the Tree to the MaybeTree class as this uses the @classmethod properly by returning the initialization of the for cls argument
  • Make the @classmethod that normalizes the source code before parsing the only entrypoint by removing the other @classmethod that does not normalize the source code before parsing to enforce normalization (and resolve the linked issues below)
  • Rename normalized_parse to from_source_code to match the commonly used naming for classmethods within UCX
  • Remove MaybeTree's methods walk and first_statement as those are repetitions from Tree's methods

Linked issues

Resolves #3457
Resolves #3213

Functionality

  • modified Python linting related code

Tests

  • added unit tests

@JCZuurmond JCZuurmond added migrate/code Abstract Syntax Trees and other dark magic migrate/python Pull requests that update Python code python Pull requests that update Python code labels Jan 20, 2025
@JCZuurmond JCZuurmond self-assigned this Jan 20, 2025
@JCZuurmond JCZuurmond requested a review from a team as a code owner January 20, 2025 07:39
nodes.add(node)
count += 1
assert len(nodes) == count


def test_parses_incorrectly_indented_code() -> None:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is removed as we now always parse the source code before constructing the tree

Copy link

github-actions bot commented Jan 20, 2025

✅ 175/175 passed, 1 flaky, 16 skipped, 3h57m54s total

Flaky tests:

  • 🤪 test_hiveserde_table_in_place_migration_job[migrate-external-tables-ctas] (4m39.483s)

Running from acceptance #8104

@@ -290,16 +290,3 @@ def test_lints_complex_dbutils_notebook_run() -> None:
linter = DbutilsPyLinter(CurrentSessionState())
advices = list(linter.lint(source))
assert not advices


def test_tree_maybe_parse_fails_on_jupyter_magic() -> None:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is moved to test_python_ast.py

Copy link
Contributor

@asnare asnare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nothing that prevents merging. A few comments, but more food for thought and definitely not blocking.

src/databricks/labs/ucx/source_code/python/python_ast.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/source_code/python/python_ast.py Outdated Show resolved Hide resolved
Comment on lines -190 to +193
def _walk(cls, node: NodeNG) -> Iterable[NodeNG]:
def _walk(self, node: NodeNG) -> Iterable[NodeNG]:
yield node
for child in node.get_children():
yield from cls._walk(child)
yield from self._walk(child)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can remain a class method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, maybe, it does not make sense to me though. This is a protected method thus it should not be reached from outside the class, thus losing the benefit of calling the method without initializing the class, like Tree._walk. Furthermore, it is only called from the Tree.walk method, which is a regular method already, so why should this not be a regular method?

@asnare asnare requested a review from a team January 23, 2025 16:32
@JCZuurmond JCZuurmond enabled auto-merge January 28, 2025 14:00
@JCZuurmond JCZuurmond added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit c64de1a Jan 28, 2025
7 checks passed
@JCZuurmond JCZuurmond deleted the fix/restrict-public-class-methods-on-tree branch January 28, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrate/code Abstract Syntax Trees and other dark magic migrate/python Pull requests that update Python code python Pull requests that update Python code
Projects
Archived in project
2 participants