Skip to content

Commit

Permalink
Bump astroid version, pylint version and drop our f-string workaround (
Browse files Browse the repository at this point in the history
…databrickslabs#2746)

## Changes
Our code works around a limitation of astroid < 3.3 where f-strings are
not inferred
This PR:
 - updates pylint and astroid
 - drops workarounds
 - fixes corresponding tests

### Linked issues
None

### Functionality
None

### Tests
- [x] updated unit tests

---------

Co-authored-by: Eric Vergnaud <eric.vergnaud@databricks.com>
  • Loading branch information
2 people authored and jgarciaf106 committed Sep 26, 2024
1 parent 04a4956 commit 15c5536
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 58 deletions.
11 changes: 7 additions & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ dependencies = ["databricks-sdk~=0.30",
"databricks-labs-blueprint>=0.8,<0.9",
"PyYAML>=6.0.0,<7.0.0",
"sqlglot>=25.5.0,<25.23",
"astroid>=3.2.2"]
"astroid>=3.3.1"]

[project.optional-dependencies]
pylsp = [
Expand All @@ -74,7 +74,7 @@ dependencies = [
"black~=24.3.0",
"coverage[toml]~=7.4.4",
"mypy~=1.9.0",
"pylint~=3.2.2",
"pylint~=3.3.1",
"pylint-pytest==2.0.0a0",
"databricks-labs-pylint~=0.4.0",
"databricks-labs-pytester>=0.2.1",
Expand Down Expand Up @@ -209,7 +209,7 @@ fail-under = 10.0
# ignore-list. The regex matches against paths and can be in Posix or Windows
# format. Because '\\' represents the directory delimiter on Windows systems, it
# can't be used as an escape character.
ignore-paths='^tests/unit/source_code/samples/.*$'
ignore-paths='^tests/unit/source_code/samples/.*$'

# Files or directories matching the regular expression patterns are skipped. The
# regex matches against base names, not paths. The default value ignores Emacs
Expand Down Expand Up @@ -587,7 +587,10 @@ disable = [
"fixme",
"consider-using-assignment-expr",
"logging-fstring-interpolation",
"consider-using-any-or-all"
"consider-using-any-or-all",
"too-many-positional-arguments",
"unnecessary-default-type-args",
"logging-not-lazy"
]

# Enable the message, report, category or checker with the given id(s). You can
Expand Down
18 changes: 0 additions & 18 deletions src/databricks/labs/ucx/source_code/python/python_infer.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ def _infer_values(cls, node: NodeNG) -> Iterator[Iterable[NodeNG]]:
# deal with node types that don't implement 'inferred()'
if node is Uninferable or isinstance(node, Const):
yield [node]
elif isinstance(node, JoinedStr):
yield from cls._infer_values_from_joined_string(node)
elif isinstance(node, FormattedValue):
yield from _LocalInferredValue.do_infer_values(node.value)
else:
yield from cls._safe_infer_internal(node)

Expand All @@ -91,20 +87,6 @@ def _unsafe_infer_internal(cls, node: NodeNG):
continue
yield from _LocalInferredValue.do_infer_values(inferred)

@classmethod
def _infer_values_from_joined_string(cls, node: NodeNG) -> Iterator[Iterable[NodeNG]]:
assert isinstance(node, JoinedStr)
yield from cls._infer_values_from_joined_values(node.values)

@classmethod
def _infer_values_from_joined_values(cls, nodes: list[NodeNG]) -> Iterator[Iterable[NodeNG]]:
if len(nodes) == 1:
yield from _LocalInferredValue.do_infer_values(nodes[0])
return
for firsts in _LocalInferredValue.do_infer_values(nodes[0]):
for remains in cls._infer_values_from_joined_values(nodes[1:]):
yield list(firsts) + list(remains)

def __init__(self, atoms: Iterable[NodeNG]):
self._atoms = list(atoms)

Expand Down
16 changes: 0 additions & 16 deletions tests/unit/source_code/python/test_python_infer.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,22 +88,6 @@ def test_infers_fstring_values():
assert strings == ["Hello abc, ghi!", "Hello abc, jkl!", "Hello def, ghi!", "Hello def, jkl!"]


def test_fails_to_infer_cascading_fstring_values():
# The purpose of this test is to detect a change in astroid support for f-strings
source = """
value1 = "John"
value2 = f"Hello {value1}"
value3 = f"{value2}, how are you today?"
"""
tree = Tree.parse(source)
nodes = tree.locate(Assign, [])
tree = Tree(nodes[2].value) # value of value3 = ...
values = list(InferredValue.infer_from_node(tree.node))
# for now, we simply check failure to infer!
assert any(not value.is_inferred() for value in values)
# the expected value would be ["Hello John, how are you today?"]


def test_infers_externally_defined_value():
state = CurrentSessionState()
state.named_parameters = {"my-widget": "my-value"}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@
print(len(result))
index = 10
spark.sql(f"SELECT * FROM table_{index}").collect()
# ucx[cannot-autofix-table-reference:+2:0:+2:40] Can't migrate table_name argument in 'f'SELECT * FROM {table_name}'' because its value cannot be computed
table_name = f"table_{index}"
spark.sql(f"SELECT * FROM {table_name}").collect()
# ucx[table-migrated-to-uc:+4:4:+4:20] Table old.things is migrated to brand.new.stuff in Unity Catalog
# ucx[cannot-autofix-table-reference:+3:4:+3:20] Can't migrate table_name argument in 'query' because its value cannot be computed
# ucx[table-migrated-to-uc:+3:4:+3:20] Table old.things is migrated to brand.new.stuff in Unity Catalog
table_name = f"table_{index}"
for query in ["SELECT * FROM old.things", f"SELECT * FROM {table_name}"]:
spark.sql(query).collect()
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import sys

name_1 = "whatever"
name_2 = f"{name_1}"
name_2 = some_call(name_1)
sys.path.append(f"{name_2}")
names = [f"{name_2}", name_2]
for name in names:
Expand Down
15 changes: 3 additions & 12 deletions tests/unit/source_code/test_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,24 +199,15 @@ def load_dependency(self, path_lookup: PathLookup, dependency: Dependency) -> So
]


def test_dependency_resolver_raises_problem_for_non_inferable_sys_path(simple_dependency_resolver):
def test_dependency_resolver_raises_problem_for_uninferrable_sys_path(simple_dependency_resolver):
maybe = simple_dependency_resolver.build_local_file_dependency_graph(
Path("sys-path-with-fstring.py"), CurrentSessionState()
Path("sys-path-with-uninferrable.py"), CurrentSessionState()
)
assert list(maybe.problems) == [
DependencyProblem(
code='sys-path-cannot-compute-value',
message="Can't update sys.path from f'{name_2}' because the expression cannot be computed",
source_path=Path('sys-path-with-fstring.py'),
start_line=4,
start_col=16,
end_line=4,
end_col=27,
),
DependencyProblem(
code='sys-path-cannot-compute-value',
message="Can't update sys.path from name because the expression cannot be computed",
source_path=Path('sys-path-with-fstring.py'),
source_path=Path('sys-path-with-uninferrable.py'),
start_line=7,
start_col=20,
end_line=7,
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/source_code/test_notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ def test_notebook_builds_python_dependency_graph_with_fstring_loop(mock_path_loo
container = maybe.dependency.load(mock_path_lookup)
assert container is not None
container.build_dependency_graph(graph)
expected_paths = [path, "leaf1.py", "leaf3.py"]
expected_paths = [path, "leaf1.py", "leaf2.py", "leaf3.py"]
all_paths = set(d.path for d in graph.all_dependencies)
assert all_paths == {mock_path_lookup.cwd / path for path in expected_paths}

Expand Down Expand Up @@ -279,13 +279,12 @@ def test_does_not_detect_partial_call_to_dbutils_notebook_run_in_python_code_()
assert len(nodes) == 0


def test_raises_advice_when_dbutils_notebook_run_is_too_complex() -> None:
def test_lints_complex_dbutils_notebook_run() -> None:
source = """
name1 = "John"
name2 = f"{name1}"
dbutils.notebook.run(f"Hey {name2}")
"""
linter = DbutilsPyLinter(CurrentSessionState())
advices = list(linter.lint(source))
assert len(advices) == 1
assert advices[0].code == "notebook-run-cannot-compute-value"
assert not advices

0 comments on commit 15c5536

Please sign in to comment.