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

Fix notebook sources with NotebookLinter.apply #3693

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

JCZuurmond
Copy link
Member

Changes

Add notebook fixing to NotebookLinter

  • Implement Notebook.apply
  • Call Notebook.apply from FileLiner.apply for Notebook source container
  • Remove legacy NotebookMigrator
  • Introduce PythonLinter to run apply on a AST tree
  • Allow to

Linked issues

Progresses #3514
Breaks up #3520

Functionality

  • modified existing command: databricks labs ucx migrate-local-code

Tests

  • manually tested
  • modified and added unit tests
  • modified and added integration tests

@JCZuurmond JCZuurmond added migrate/python Pull requests that update Python code python Pull requests that update Python code bug/test-infra issues related to testing infrastructure labels Feb 14, 2025
@JCZuurmond JCZuurmond self-assigned this Feb 14, 2025
Copy link
Member Author

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

PR is in draft as it is blocked by fixes required in the underlying linters

tests/unit/source_code/linters/test_files.py Show resolved Hide resolved
@@ -303,18 +304,36 @@ def test_notebook_linter_lints_source_yielding_parse_failures(migration_index, m
]


def test_notebook_linter_lints_migrated_table(migration_index, mock_path_lookup) -> None:
def test_notebook_linter_lints_and_applies_migrated_table(tmp_path, migration_index, mock_path_lookup) -> 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.

This test fails because the underlying linter does not support fixing the code yet when the value should be inferred instead of being a constant in the table call


def test_notebook_linter_lints_migrated_table_with_rename(migration_index, mock_path_lookup) -> None:

def test_notebook_linter_lints_and_applies_migrated_table_with_rename(
Copy link
Member Author

Choose a reason for hiding this comment

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

This test fails because the underlying linter does not support fixing the code yet when the value should be inferred instead of being a constant in the table call

@JCZuurmond
Copy link
Member Author

Unblocking this PR and tackling the issue in seperately: #3695

@JCZuurmond JCZuurmond force-pushed the feat/add-notebook-fixing branch from 2663c6a to 7b63701 Compare February 14, 2025 11:16
@JCZuurmond JCZuurmond marked this pull request as ready for review February 14, 2025 11:16
@JCZuurmond JCZuurmond requested a review from a team as a code owner February 14, 2025 11:16
@JCZuurmond JCZuurmond force-pushed the feat/add-notebook-fixing branch from 7b63701 to 5677dad Compare February 14, 2025 11:19
Copy link

github-actions bot commented Feb 14, 2025

✅ 81/81 passed, 2 skipped, 41m47s total

Running from acceptance #8346

src/databricks/labs/ucx/source_code/linters/pyspark.py Outdated Show resolved Hide resolved
"https://github.com/databrickslabs/ucx/issues/new?title=Autofix+the+following+Python+code"
"&labels=migrate/code,needs-triage&type=Feature"
"&body=%23+Desired+behaviour%0A%0AAutofix+following+Python+code"
"%0A%0A%60%60%60+python%0ATODO:+Add+relevant+source+code%0A"
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a link to doc 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.

Could you clarify this change request? What is a "documentation method"?

Copy link
Contributor

@pritishpai pritishpai left a comment

Choose a reason for hiding this comment

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

LGTM! Merge after Liran's requested changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/test-infra issues related to testing infrastructure migrate/python Pull requests that update Python code python Pull requests that update Python code
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

3 participants