Align DependencyProblem with LocatedAdvice (#3583)

## Changes
While graph building for linting, the `DependencyProblem` are converted
to `LocatedAdvice`. This PR aligns the two classes

### Linked issues

Progresses #3514
Breaks up #3520

### Functionality

- [x] modified existing command: `databricks labs ucx
migrate-local-code`

### Tests

- [ ] manually tested
- [x] modified and added unit tests
- [x] modified and added integration tests

Author: JCZuurmond

src/databricks/labs/ucx/source_code/base.py

@@ -90,26 +90,20 @@ def replace_from_node(self, node: NodeNG) -> Advice:
 
 @dataclass
 class LocatedAdvice:
+    """The advice with a path location."""
+
     advice: Advice
+    """The advice"""
+
     path: Path
+    """The path location"""
 
-    @property
-    def is_unknown(self) -> bool:
-        return self.path == Path('UNKNOWN')
-
-    def message_relative_to(self, base: Path, *, default: Path | None = None) -> str:
-        advice = self.advice
-        path = self.path
-        if self.is_unknown:
-            logger.debug(f'THIS IS A BUG! {advice.code}:{advice.message} has unknown path')
-        if default is not None:
-            path = default
-        try:
-            path = path.relative_to(base)
-        except ValueError:
-            logger.debug(f'Not a relative path: {path} to base: {base}')
-        # increment start_line because it is 0-based whereas IDEs are usually 1-based
-        return f"./{path.as_posix()}:{advice.start_line+1}:{advice.start_col}: [{advice.code}] {advice.message}"
+    def __str__(self) -> str:
+        return f"{self.path.as_posix()}:{self.advice.start_line + 1}:{self.advice.start_col}: [{self.advice.code}] {self.advice.message}"
+
+    def has_missing_path(self) -> bool:
+        """Flag if the path is missing, or not."""
+        return self.path == Path("<MISSING_SOURCE_PATH>")  # Reusing flag from DependencyProblem
 
 
 class Advisory(Advice):

src/databricks/labs/ucx/source_code/graph.py

@@ -12,7 +12,13 @@
 from astroid import (  # type: ignore
     NodeNG,
 )
-from databricks.labs.ucx.source_code.base import Advisory, CurrentSessionState, is_a_notebook, LineageAtom
+from databricks.labs.ucx.source_code.base import (
+    Advisory,
+    CurrentSessionState,
+    LineageAtom,
+    LocatedAdvice,
+    is_a_notebook,
+)
 from databricks.labs.ucx.source_code.python.python_ast import Tree
 from databricks.labs.ucx.source_code.path_lookup import PathLookup
 
@@ -93,7 +99,7 @@ def register_dependency(self, dependency: Dependency) -> MaybeGraph:
             return MaybeGraph(child_graph, [problem])
         problems = []
         for problem in container.build_dependency_graph(child_graph):
-            if problem.is_path_missing():
+            if problem.has_missing_path():
                 problem = dataclasses.replace(problem, source_path=dependency.path)
             problems.append(problem)
         return MaybeGraph(child_graph, problems)
@@ -467,7 +473,7 @@ def build_notebook_dependency_graph(self, path: Path, session_state: CurrentSess
     def _make_relative_paths(self, problems: list[DependencyProblem], path: Path) -> list[DependencyProblem]:
         adjusted_problems = []
         for problem in problems:
-            out_path = path if problem.is_path_missing() else problem.source_path
+            out_path = path if problem.has_missing_path() else problem.source_path
             if out_path.is_absolute() and out_path.is_relative_to(self._path_lookup.cwd):
                 out_path = out_path.relative_to(self._path_lookup.cwd)
             adjusted_problems.append(dataclasses.replace(problem, source_path=out_path))
@@ -482,27 +488,49 @@ def __repr__(self):
 
 @dataclass
 class DependencyProblem:
+    """A problem found with the dependency.
+
+    The line and column indexing is 0-based.
+    """
+
     code: str
+    """Unique code to identify the problem type."""
+
     message: str
+    """A message detailing the problem."""
+
     source_path: Path = _MISSING_SOURCE_PATH
-    # Lines and columns are both 0-based: the first line is line 0.
+    """The path to the problem"""
+
     start_line: int = -1
+    """The line where the problem starts when reading source code."""
+
     start_col: int = -1
+    """The column where the problem starts when reading source code."""
+
     end_line: int = -1
+    """The line where the problem ends when reading source code."""
+
     end_col: int = -1
+    """The column where the problem ends when reading source code."""
 
-    def is_path_missing(self) -> bool:
+    def has_missing_path(self) -> bool:
+        """Flag if the path is missing, or not."""
         return self.source_path == _MISSING_SOURCE_PATH
 
-    def as_advisory(self) -> 'Advisory':
-        return Advisory(
+    def as_located_advice(self) -> LocatedAdvice:
+        """Converts the dependency problem in a located advice for linting."""
+        # Advisory level is chosen to treat a dependency problem as a WARNING. It is the most server while not being
+        # CRITICAL (yet).
+        advisory = Advisory(
             code=self.code,
             message=self.message,
             start_line=self.start_line,
             start_col=self.start_col,
             end_line=self.end_line,
             end_col=self.end_col,
         )
+        return LocatedAdvice(advisory, self.source_path)
 
     @staticmethod
     def from_node(code: str, message: str, node: NodeNG) -> DependencyProblem:

src/databricks/labs/ucx/source_code/jobs.py

@@ -474,7 +474,7 @@ def _lint_job(self, job: jobs.Job) -> tuple[list[JobProblem], list[DirectFsAcces
             if not advices:
                 advices = self._lint_task(task, graph, session_state, linted_paths)
             for advice in advices:
-                absolute_path = advice.path.absolute().as_posix() if advice.path != self._UNKNOWN else 'UNKNOWN'
+                absolute_path = "UNKNOWN" if advice.has_missing_path() else advice.path.absolute().as_posix()
                 job_problem = JobProblem(
                     job_id=job.job_id,
                     job_name=job.settings.name,
@@ -521,10 +521,7 @@ def _build_task_dependency_graph(
         )
         graph = DependencyGraph(root_dependency, None, self._resolver, self._path_lookup, session_state)
         problems = container.build_dependency_graph(graph)
-        located_advices: list[LocatedAdvice] = []
-        for problem in problems:
-            source_path = self._UNKNOWN if problem.is_path_missing() else problem.source_path
-            located_advices.append(LocatedAdvice(problem.as_advisory(), source_path))
+        located_advices = [problem.as_located_advice() for problem in problems]
         return graph, located_advices, session_state
 
     def _lint_task(

src/databricks/labs/ucx/source_code/linters/files.py

@@ -53,7 +53,7 @@ def build_dependency_graph(self, parent: DependencyGraph) -> list[DependencyProb
             analyzer = PythonCodeAnalyzer(context, self._original_code)
             problems = analyzer.build_graph()
             for idx, problem in enumerate(problems):
-                if problem.is_path_missing():
+                if problem.has_missing_path():
                     problems[idx] = dataclasses.replace(problem, source_path=self._path)
             return problems
         # supported language that does not generate dependencies
@@ -69,7 +69,7 @@ def build_inherited_context(self, graph: DependencyGraph, child_path: Path) -> I
             inherited = analyzer.build_inherited_context(child_path)
             problems = list(inherited.problems)
             for idx, problem in enumerate(problems):
-                if problem.is_path_missing():
+                if problem.has_missing_path():
                     problems[idx] = dataclasses.replace(problem, source_path=self._path)
             return dataclasses.replace(inherited, problems=problems)
         return InheritedContext(None, False, [])
@@ -150,9 +150,8 @@ def lint(
             )
             path = Path(response)
         located_advices = list(self.lint_path(path))
-        for located in located_advices:
-            message = located.message_relative_to(path)
-            stdout.write(f"{message}\n")
+        for located_advice in located_advices:
+            stdout.write(f"{located_advice}\n")
         return located_advices
 
     def lint_path(self, path: Path, linted_paths: set[Path] | None = None) -> Iterable[LocatedAdvice]:
@@ -171,8 +170,7 @@ def lint_path(self, path: Path, linted_paths: set[Path] | None = None) -> Iterab
         assert container is not None  # because we just created it
         problems = container.build_dependency_graph(graph)
         for problem in problems:
-            problem_path = Path('UNKNOWN') if problem.is_path_missing() else problem.source_path.absolute()
-            yield problem.as_advisory().for_path(problem_path)
+            yield problem.as_located_advice()
         context_factory = self._context_factory
         session_state = self._session_state
 

tests/integration/source_code/solacc.py

@@ -80,10 +80,8 @@ def _collect_unparseable(advices: list[LocatedAdvice]):
     return list(located_advice for located_advice in advices if located_advice.advice.code == 'parse-error')
 
 
-def _print_advices(advices: list[LocatedAdvice]):
-    messages = list(
-        located_advice.message_relative_to(dist.parent).replace('\n', ' ') + '\n' for located_advice in advices
-    )
+def _print_advices(located_advices: list[LocatedAdvice]) -> None:
+    messages = [f"{located_advice}\n" for located_advice in located_advices]
     if os.getenv("CI"):
         advices_path = build / "advices.txt"
         with advices_path.open("a") as advices_file:

tests/integration/source_code/test_graph.py

@@ -1,9 +1,8 @@
-import dataclasses
 from pathlib import Path
 
 
 from databricks.labs.ucx.source_code.base import CurrentSessionState
-from databricks.labs.ucx.source_code.graph import DependencyResolver, DependencyGraph, DependencyProblem
+from databricks.labs.ucx.source_code.graph import DependencyResolver, DependencyGraph
 from databricks.labs.ucx.source_code.known import KnownList, Compatibility, UNKNOWN
 from databricks.labs.ucx.source_code.linters.files import FileLoader, ImportFileResolver
 from databricks.labs.ucx.source_code.notebooks.loaders import NotebookLoader, NotebookResolver
@@ -58,10 +57,3 @@ def test_graph_imports_dynamic_import():
     container = maybe.dependency.load(path_lookup)
     problems = container.build_dependency_graph(graph)
     assert not problems
-
-
-def test_is_path_missing():
-    problem = DependencyProblem("some-code", "some-message")
-    assert problem.is_path_missing()
-    problem = dataclasses.replace(problem, source_path=Path("stuff"))
-    assert not problem.is_path_missing()

tests/unit/source_code/test_base.py

@@ -1,4 +1,5 @@
 import dataclasses
+from pathlib import Path
 
 import pytest
 
@@ -7,6 +8,7 @@
     Advisory,
     Convention,
     Deprecation,
+    LocatedAdvice,
     Failure,
     UsedTable,
 )
@@ -54,3 +56,17 @@ def test_convention_initialization() -> None:
 )
 def test_used_table_full_name(used_table: UsedTable, expected_name: str) -> None:
     assert used_table.full_name == expected_name
+
+
+def test_located_advice_message() -> None:
+    advice = Advice(
+        code="code",
+        message="message",
+        start_line=0,
+        start_col=0,  # Zero based line number is incremented with one to create the message
+        end_line=1,
+        end_col=1,
+    )
+    located_advice = LocatedAdvice(advice, Path("test.py"))
+
+    assert str(located_advice) == "test.py:1:0: [code] message"

tests/unit/source_code/test_graph.py

@@ -3,11 +3,12 @@
 
 import pytest
 
-from databricks.labs.ucx.source_code.base import CurrentSessionState
+from databricks.labs.ucx.source_code.base import Advisory, CurrentSessionState, LocatedAdvice
 from databricks.labs.ucx.source_code.linters.files import FileLoader, FolderLoader
 from databricks.labs.ucx.source_code.graph import (
     Dependency,
     DependencyGraph,
+    DependencyProblem,
     InheritedContext,
     DependencyGraphWalker,
 )
@@ -217,3 +218,25 @@ def _process_dependency(
 
     walker = _TestWalker(root_graph, set(), mock_path_lookup)
     _ = list(_ for _ in walker)
+
+
+def test_dependency_problem_has_path_missing_by_default() -> None:
+    problem = DependencyProblem("code", "message")
+    assert problem.has_missing_path()
+
+
+def test_dependency_problem_has_path_when_given() -> None:
+    problem = DependencyProblem("code", "message", source_path=Path("location"))
+    assert not problem.has_missing_path()
+
+
+def test_dependency_problem_as_located_advice() -> None:
+    problem = DependencyProblem("code", "message")
+    located_advice = problem.as_located_advice()
+    assert located_advice == LocatedAdvice(Advisory("code", "message", -1, -1, -1, -1), problem.source_path)
+
+
+def test_dependency_problem_as_located_advice_has_path_missing_by_default() -> None:
+    problem = DependencyProblem("code", "message")
+    located_advice = problem.as_located_advice()
+    assert located_advice.has_missing_path()

tests/unit/source_code/test_jobs.py

@@ -246,7 +246,7 @@ def test_workflow_linter_lint_job_logs_problems(dependency_resolver, mock_path_l
 
     directfs_crawler.assert_not_called()
     used_tables_crawler.assert_not_called()
-    assert any(message.startswith(expected_message) for message in caplog.messages)
+    assert any(message.startswith(expected_message) for message in caplog.messages), caplog.messages
 
 
 def test_workflow_task_container_builds_dependency_graph_for_requirements_txt(mock_path_lookup, graph) -> None: