Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion justfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ venv := ".venv"
bin := venv + "/bin"
python_version := "python3.13"
run := "poetry run"
target_dirs := "src tests"
target_dirs := "src tests scripts"
image := "docker.io/elementsinteractive/lgtm-ai"

# SENTINELS
Expand Down
4 changes: 4 additions & 0 deletions lgtm.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ model = "gemini-2.5-pro"
silent = false
publish = true
ai_retries = 2

# GitHub issues integration
issues_platform = "github"
issues_url = "https://github.com/elementsinteractive/lgtm-ai/issues"
4 changes: 2 additions & 2 deletions scripts/evaluate_review_quality.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 TargetParser
from rich.logging import RichHandler

PRS_FOR_EVALUATION = {
Expand Down Expand Up @@ -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 = TargetParser(allow_git_repo=True)(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(),
Expand Down
5 changes: 3 additions & 2 deletions src/lgtm_ai/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from lgtm_ai.validators import (
IntOrNoLimitType,
ModelChoice,
parse_target,
TargetParser,
validate_model_url,
)
from rich.console import Console
Expand All @@ -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),
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
16 changes: 14 additions & 2 deletions src/lgtm_ai/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
9 changes: 0 additions & 9 deletions tests/config/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from collections.abc import Iterator
from contextlib import contextmanager
from pathlib import Path
from unittest import mock

import pytest

Expand Down Expand Up @@ -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
13 changes: 13 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
23 changes: 18 additions & 5 deletions tests/test_main.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
from pathlib import Path
from unittest import mock

import click
Expand Down Expand Up @@ -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,
Expand All @@ -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,
)
Expand Down Expand Up @@ -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(
Expand Down
13 changes: 9 additions & 4 deletions tests/test_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down