fix: handling of target for guides, tests using lgtm.toml

Author: scastlara

scripts/evaluate_review_quality.py

@@ -15,7 +15,7 @@
 from lgtm_ai.git_client.gitlab import GitlabClient
 from lgtm_ai.review import CodeReviewer
 from lgtm_ai.review.context import ContextRetriever
-from lgtm_ai.validators import parse_target
+from lgtm_ai.validators import _parse_target
 from rich.logging import RichHandler
 
 PRS_FOR_EVALUATION = {
@@ -94,7 +94,7 @@ def get_git_branch() -> str:
 def perform_review(
     output_dir: str, pr_url: str, pr_name: str, sample: int, model: SupportedAIModels, git_api_key: str, ai_api_key: str
 ) -> None:
-    url = parse_target(mock.Mock(), "pr_url", pr_url)
+    url = _parse_target(mock.Mock(), "pr_url", pr_url)
     git_client = GitlabClient(client=gitlab.Gitlab(private_token=git_api_key), formatter=MarkDownFormatter())
     code_reviewer = CodeReviewer(
         reviewer_agent=get_reviewer_agent_with_settings(),

src/lgtm_ai/__main__.py

@@ -32,7 +32,7 @@
 from lgtm_ai.validators import (
     IntOrNoLimitType,
     ModelChoice,
-    parse_target,
+    TargetParser,
     validate_model_url,
 )
 from rich.console import Console
@@ -57,7 +57,6 @@ def entry_point() -> None:
 def _common_options[**P, T](func: Callable[P, T]) -> Callable[P, T]:
     """Wrap a click command and adds common options for lgtm commands."""
 
-    @click.argument("target", required=True, callback=parse_target)
     @click.option(
         "--model",
         type=ModelChoice(SupportedAIModelsList),
@@ -112,6 +111,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T:
     return wrapper
 
 
+@click.argument("target", required=True, callback=TargetParser(allow_git_repo=True))
 @entry_point.command()
 @_common_options
 @click.option(
@@ -216,6 +216,7 @@ def review(target: PRUrl | LocalRepository, config: str | None, verbose: int, **
         logger.info("Review published successfully")
 
 
+@click.argument("target", required=True, callback=TargetParser(allow_git_repo=False))
 @entry_point.command()
 @_common_options
 def guide(

src/lgtm_ai/validators.py

@@ -16,7 +16,17 @@ class AllowedSchemes(StrEnum):
     Http = "http"
 
 
-def parse_target(ctx: click.Context, param: str, value: object) -> PRUrl | LocalRepository:
+class TargetParser:
+    """Generate a click callback that parses the `TARGET` argument."""
+
+    def __init__(self, allow_git_repo: bool) -> None:
+        self.allow_git_repo = allow_git_repo
+
+    def __call__(self, ctx: click.Context, param: str, value: object) -> PRUrl | LocalRepository:
+        return _parse_target(ctx, param, value, allow_git_repo=self.allow_git_repo)
+
+
+def _parse_target(ctx: click.Context, param: str, value: object, *, allow_git_repo: bool) -> PRUrl | LocalRepository:
     """Click callback that transforms a given URL into a dataclass for later use.
 
     It validates it and raises click exceptions if the URL is not valid.
@@ -26,7 +36,9 @@ def parse_target(ctx: click.Context, param: str, value: object) -> PRUrl | Local
 
     parsed = urlparse(value)
     if not parsed.netloc:
-        # Check whether its a directory path
+        if not allow_git_repo:
+            raise click.BadParameter("The PR URL must be a valid URL")
+
         try:
             resolved_path = pathlib.Path(value).resolve(strict=True)  # just to ensure it's a valid path
             # Check whether it's a git repository

tests/config/conftest.py

@@ -3,7 +3,6 @@
 from collections.abc import Iterator
 from contextlib import contextmanager
 from pathlib import Path
-from unittest import mock
 
 import pytest
 
@@ -144,11 +143,3 @@ def clean_env_secrets() -> Iterator[None]:
         # Restore original environment
         os.environ.clear()
         os.environ.update(original_env)
-
-
-@pytest.fixture(autouse=True)
-def mock_current_working_directory(tmp_path: Path) -> Iterator[None]:
-    with mock.patch("lgtm_ai.config.handler.os.getcwd", return_value=str(tmp_path)):
-        # We still mock the current directory because this is a python project,
-        # and thus it has a pyproject.toml file that will be read during the test execution!
-        yield

tests/conftest.py

@@ -1,10 +1,23 @@
+from collections.abc import Iterator
 from copy import deepcopy
+from pathlib import Path
 from typing import Any
+from unittest import mock
 from unittest.mock import MagicMock
 
 import pytest
 
 
+@pytest.fixture(autouse=True)
+def mock_current_working_directory(tmp_path: Path) -> Iterator[None]:
+    """Mock the current working directory for config handling in all tests.
+
+    This is done so that the `lgtm.toml` file of `lgtm` is not used in tests.
+    """
+    with mock.patch("lgtm_ai.config.handler.os.getcwd", return_value=str(tmp_path)):
+        yield
+
+
 class CopyingMock(MagicMock):
     """Mock which copies arguments when mocks are called with mutable arguments.
 

tests/test_main.py

@@ -1,4 +1,5 @@
 import logging
+from pathlib import Path
 from unittest import mock
 
 import click
@@ -73,7 +74,14 @@ def test_review_cli_github(*args: mock.MagicMock) -> None:
 @mock.patch("lgtm_ai.__main__.CodeReviewer")
 @mock.patch("lgtm_ai.__main__.PrettyFormatter")
 @mock.patch("lgtm_ai.__main__.get_git_client")
-def test_review_cli_local(*args: mock.MagicMock) -> None:
+def test_review_cli_local(
+    m_get_git_client: mock.MagicMock,
+    m_formatter: mock.MagicMock,
+    m_reviewer: mock.MagicMock,
+    tmp_path: Path,
+) -> None:
+    # Create a .git directory to simulate a git repository
+    (tmp_path / ".git").mkdir()
     runner = CliRunner()
     result = runner.invoke(
         review,
@@ -90,14 +98,19 @@ def test_review_cli_local(*args: mock.MagicMock) -> None:
 @mock.patch("lgtm_ai.__main__.CodeReviewer")
 @mock.patch("lgtm_ai.__main__.PrettyFormatter")
 @mock.patch("lgtm_ai.__main__.get_git_client")
-def test_review_cli_local_does_not_exist(*args: mock.MagicMock) -> None:
+def test_review_cli_local_does_not_exist(
+    m_get_git_client: mock.MagicMock,
+    m_formatter: mock.MagicMock,
+    m_reviewer: mock.MagicMock,
+    tmp_path: Path,
+) -> None:
     runner = CliRunner()
     result = runner.invoke(
         review,
         [
             "--ai-api-key",
             "fake-token",
-            "./foo/bar/baz",
+            "./fake",
         ],
         catch_exceptions=False,
     )
@@ -162,8 +175,8 @@ def test_guide_cli_local_fails(*args: mock.MagicMock) -> None:
         catch_exceptions=False,
     )
 
-    assert result.exit_code == 1
-    assert "Aborted!" in result.stderr
+    assert result.exit_code == 2
+    assert "Invalid value for 'TARGET': The PR URL must be a valid URL" in result.stderr
 
 
 @pytest.mark.parametrize(

tests/test_validators.py

@@ -6,7 +6,7 @@
 import click
 import pytest
 from lgtm_ai.base.schemas import PRSource, PRUrl
-from lgtm_ai.validators import parse_target, validate_model_url
+from lgtm_ai.validators import TargetParser, validate_model_url
 
 
 @pytest.mark.parametrize(
@@ -34,12 +34,17 @@
 )
 def test_parse_url(url: str, expectation: AbstractContextManager[Any]) -> None:
     with expectation:
-        parse_target(mock.Mock(), "pr_url", url)
+        TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url)
+
+
+def test_parse_url_does_not_accept_git_repo() -> None:
+    with pytest.raises(click.exceptions.BadParameter, match="The PR URL must be a valid URL"):
+        TargetParser(allow_git_repo=False)(mock.Mock(), "pr_url", "./foo/bar/baz")
 
 
 def test_parse_url_gitlab_valid() -> None:
     url = "https://gitlab.com/foo/-/merge_requests/1"
-    parsed = parse_target(mock.Mock(), "pr_url", url)
+    parsed = TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url)
 
     assert parsed == PRUrl(
         full_url=url,
@@ -52,7 +57,7 @@ def test_parse_url_gitlab_valid() -> None:
 
 def test_parse_url_github_valid() -> None:
     url = "https://github.com/elementsinteractive/sheriff/pull/61"
-    parsed = parse_target(mock.Mock(), "pr_url", url)
+    parsed = TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url)
 
     assert parsed == PRUrl(
         full_url=url,