fix(#32): relative line number for GitHub was incorrect

Author: scastlara

src/lgtm_ai/git_parser/parser.py

@@ -33,15 +33,15 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: str) -> DiffResult:
 
     old_line_num = 0
     new_line_num = 0
-    rel_position = 1
+    rel_position = -1  # We just don't count the first hunk, but we do count the rest
 
     try:
         for line in lines:
             hunk_match = re.match(r"^@@ -(\d+),?\d* \+(\d+),?\d* @@", line)
+            rel_position += 1
             if hunk_match:
                 old_line_num = int(hunk_match.group(1))
                 new_line_num = int(hunk_match.group(2))
-                rel_position = 1  # Reset position for each hunk
                 continue
 
             if line.startswith("+") and not line.startswith("+++"):
@@ -69,7 +69,6 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: str) -> DiffResult:
             else:
                 old_line_num += 1
                 new_line_num += 1
-            rel_position += 1
     except (ValueError, TypeError, KeyError) as err:
         raise GitDiffParseError("Failed to parse diff patch") from err
 

tests/git_client/test_github.py

@@ -131,13 +131,13 @@ def test_get_diff_from_url_successful() -> None:
                     ModifiedLine(
                         line='    {{ run }} ruff check {{ target_dirs }} {{ if report == "true" { "--format gitlab > tests/gl-code-quality-report.json" } else { "" } }}',
                         line_number=48,
-                        relative_line_number=1,
+                        relative_line_number=5,
                         modification_type="removed",
                     ),
                     ModifiedLine(
                         line='    {{ run }} ruff check {{ target_dirs }} {{ if report == "true" { "--output-format gitlab > tests/gl-code-quality-report.json" } else { "" } }}',
                         line_number=48,
-                        relative_line_number=2,
+                        relative_line_number=6,
                         modification_type="added",
                     ),
                 ],
@@ -154,13 +154,13 @@ def test_get_diff_from_url_successful() -> None:
                     ModifiedLine(
                         line="[tool.ruff.per-file-ignores]",
                         line_number=78,
-                        relative_line_number=1,
+                        relative_line_number=5,
                         modification_type="removed",
                     ),
                     ModifiedLine(
                         line="[tool.ruff.lint.per-file-ignores]",
                         line_number=78,
-                        relative_line_number=2,
+                        relative_line_number=6,
                         modification_type="added",
                     ),
                 ],

tests/git_parser/fixtures.py

@@ -57,3 +57,128 @@
         ModifiedLine(line="        total += price", line_number=12, relative_line_number=5, modification_type="added"),
     ],
 )
+
+COMPLEX_DIFF_TEXT = '''
+'@@ -2,7 +2,7 @@
+ import logging
+ from collections.abc import Callable
+ from importlib.metadata import version
+-from typing import get_args
++from typing import Any, assert_never, get_args
+
+ import click
+ import rich
+@@ -13,10 +13,12 @@
+     get_summarizing_agent_with_settings,
+ )
+ from lgtm_ai.ai.schemas import AgentSettings, CommentCategory, SupportedAIModels, SupportedAIModelsList
+-from lgtm_ai.base.schemas import PRUrl
++from lgtm_ai.base.schemas import OutputFormat, PRUrl
+ from lgtm_ai.config.handler import ConfigHandler, PartialConfig
++from lgtm_ai.formatters.base import Formatter
++from lgtm_ai.formatters.json import JsonFormatter
+ from lgtm_ai.formatters.markdown import MarkDownFormatter
+-from lgtm_ai.formatters.terminal import TerminalFormatter
++from lgtm_ai.formatters.pretty import PrettyFormatter
+ from lgtm_ai.git_client.utils import get_git_client
+ from lgtm_ai.review import CodeReviewer
+ from lgtm_ai.review.guide import ReviewGuideGenerator
+@@ -25,14 +27,15 @@
+     parse_pr_url,
+     validate_model_url,
+ )
++from rich.console import Console
+ from rich.logging import RichHandler
+
+ __version__ = version("lgtm-ai")
+
+ logging.basicConfig(
+     format="%(message)s",
+     datefmt="[%X]",
+-    handlers=[RichHandler(rich_tracebacks=True, show_path=False)],
++    handlers=[RichHandler(rich_tracebacks=True, show_path=False, console=Console(stderr=True))],
+ )
+ logger = logging.getLogger("lgtm")
+
+@@ -68,6 +71,7 @@ def _common_options[**P, T](func: Callable[P, T]) -> Callable[P, T]:
+         help="Exclude files from the review. If not provided, all files in the PR will be reviewed. Uses UNIX-style wildcards.",
+     )
+     @click.option("--publish", is_flag=True, help="Publish the review or guide to the git service.")
++    @click.option("--output-format", type=click.Choice([format.value for format in OutputFormat]))
+     @click.option("--silent", is_flag=True, help="Do not print the review or guide to the console.")
+     @click.option(
+         "--ai-retries",
+@@ -104,6 +108,7 @@ def review(
+     config: str | None,
+     exclude: tuple[str, ...],
+     publish: bool,
++    output_format: OutputFormat | None,
+     silent: bool,
+     ai_retries: int | None,
+     verbose: int,
+@@ -125,6 +130,7 @@ def review(
+             model=model,
+             model_url=model_url,
+             publish=publish,
++            output_format=output_format,
+             silent=silent,
+             ai_retries=ai_retries,
+         ),
+@@ -146,10 +152,10 @@ def review(
+
+     if not resolved_config.silent:
+         logger.info("Printing review to console")
+-        terminal_formatter = TerminalFormatter()
+-        rich.print(terminal_formatter.format_review_summary_section(review))
++        formatter, printer = _get_formatter_and_printer(resolved_config.output_format)
++        printer(formatter.format_review_summary_section(review))
+         if review.review_response.comments:
+-            rich.print(terminal_formatter.format_review_comments_section(review.review_response.comments))
++            printer(formatter.format_review_comments_section(review.review_response.comments))
+
+     if resolved_config.publish:
+         logger.info("Publishing review to git service")
+@@ -168,6 +174,7 @@ def guide(
+     config: str | None,
+     exclude: tuple[str, ...],
+     publish: bool,
++    output_format: OutputFormat | None,
+     silent: bool,
+     ai_retries: int | None,
+     verbose: int,
+@@ -184,6 +191,7 @@ def guide(
+             ai_api_key=ai_api_key,
+             model=model,
+             publish=publish,
++            output_format=output_format,
+             silent=silent,
+             ai_retries=ai_retries,
+         ),
+@@ -203,8 +211,8 @@ def guide(
+
+     if not resolved_config.silent:
+         logger.info("Printing review to console")
+-        terminal_formatter = TerminalFormatter()
+-        rich.print(terminal_formatter.format_guide(guide))
++        formatter, printer = _get_formatter_and_printer(resolved_config.output_format)
++        printer(formatter.format_guide(guide))
+
+     if resolved_config.publish:
+         logger.info("Publishing review guide to git service")
+@@ -220,3 +228,15 @@ def _set_logging_level(logger: logging.Logger, verbose: int) -> None:
+     else:
+         logger.setLevel(logging.DEBUG)
+     logger.info("Logging level set to %s", logging.getLevelName(logger.level))
++
++
++def _get_formatter_and_printer(output_format: OutputFormat) -> tuple[Formatter[Any], Callable[[Any], None]]:
++    """Get the formatter and the print method based on the output format."""
++    if output_format == OutputFormat.pretty:
++        return PrettyFormatter(), rich.print
++    elif output_format == OutputFormat.markdown:
++        return MarkDownFormatter(), print
++    elif output_format == OutputFormat.json:
++        return JsonFormatter(), print
++    else:
++        assert_never(output_format)'
+'''

tests/git_parser/test_parser.py

@@ -1,6 +1,7 @@
 import pytest
 from lgtm_ai.git_parser.parser import DiffResult, parse_diff_patch
 from tests.git_parser.fixtures import (
+    COMPLEX_DIFF_TEXT,
     DUMMY_METADATA,
     PARSED_REFACTOR_DIFF,
     PARSED_SIMPLE_DIFF,
@@ -18,3 +19,20 @@
 )
 def test_parse_diff_patch(input_diff: str, expected: DiffResult) -> None:
     assert parse_diff_patch(DUMMY_METADATA, input_diff) == expected
+
+
+@pytest.mark.parametrize(
+    ("content", "expected_line"),
+    [
+        ("from typing import get_args", 4),
+        ("printer(formatter.format_review_comments_section(review.review_response.comments))", 75),
+        ("assert_never(output_format)", 121),
+    ],
+)
+def test_parse_diff_relative_line_multiple_hunks(content: str, expected_line: int) -> None:
+    parsed = parse_diff_patch(DUMMY_METADATA, COMPLEX_DIFF_TEXT)
+
+    parsed_line = next(parsed_line for parsed_line in parsed.modified_lines if content in parsed_line.line)
+    assert parsed_line.relative_line_number == expected_line, (
+        f"Expected line number {expected_line}, got {parsed_line.line_number}"
+    )