-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
@@ -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: |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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
Unblocking this PR and tackling the issue in seperately: #3695 |
2663c6a
to
7b63701
Compare
7b63701
to
5677dad
Compare
✅ 81/81 passed, 2 skipped, 41m47s total Running from acceptance #8346 |
"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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"?
5f54c81
to
dc46932
Compare
There was a problem hiding this 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
Explain why the PythonFixer works like this
bb7fb9e
to
6042657
Compare
Changes
Add notebook fixing to
NotebookLinter
Notebook.apply
Notebook.apply
fromFileLiner.apply
forNotebook
source containerNotebookMigrator
PythonLinter
to run apply on a AST treeLinked issues
Progresses #3514
Breaks up #3520
Functionality
databricks labs ucx migrate-local-code
Tests