feat(#18): add json formatter and new `--output-format` option and config by scastlara · Pull Request #30 · elementsinteractive/lgtm-ai

scastlara · 2025-05-29T09:42:30Z

In order for this to work I had to:

Change all models to be pydantic models, instead of the mix of dataclasses and pydantic models we had before.
Separate logs to stderr and output to stdout. The default behavior of rich is quite surprising!
Renamed TerminalFormatter to PrettyFormatter.

closes #18

…nfig

scastlara · 2025-05-29T09:44:02Z

/lgtm review

github-actions

🦉 lgtm Review

Score: LGTM 👍

🔍 Summary

This is a well-executed PR that successfully adds the JSON output format and standardizes data models using Pydantic. The changes are logical, well-implemented, and include appropriate tests. The overall code quality is high. A minor refinement is suggested for the CLI's print logic when using the JSON format to enhance clarity, but this does not detract significantly from the PR's quality. The PR is generally ready for merge.

More information

Id: 103526e398814de78b995bc082ea86ae
Model: gemini-2.5-pro-preview-05-06
Created at: 2025-05-29T09:47:29.129038+00:00

Usage summary

Call 1

Request count: 1
Request tokens: 48632
Response tokens: 411
Total tokens: 918061

Call 2

Request count: 1
Request tokens: 20571
Response tokens: 422
Total tokens: 697895

Total tokens: 1615956

See the 📚 lgtm-ai repository for more information about lgtm.

scastlara

TESTING LINE NUM

scastlara

TESTING LINE NUM

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING LINE NUM 74

scastlara

TESTING MULTIPLES

scastlara · 2025-05-29T11:31:02Z

src/lgtm_ai/__main__.py

        logger.setLevel(logging.DEBUG)
    logger.info("Logging level set to %s", logging.getLevelName(logger.level))
+
+


Testing comment at line 111 with content:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

    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]]:


Testing comment at line 112 with content:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+
+
+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."""


Testing comment at line 113 with content: def _get_formatter_and_printer(output_format: OutputFormat) -> tuple[Formatter[Any], Callable[[Any], None]]:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+
+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:


Testing comment at line 114 with content: """Get the formatter and the print method based on the output format."""

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+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


Testing comment at line 115 with content: if output_format == OutputFormat.pretty:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+    if output_format == OutputFormat.pretty:
+        return PrettyFormatter(), rich.print
+    elif output_format == OutputFormat.markdown:
+        return MarkDownFormatter(), print


Testing comment at line 117 with content: elif output_format == OutputFormat.markdown:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+        return PrettyFormatter(), rich.print
+    elif output_format == OutputFormat.markdown:
+        return MarkDownFormatter(), print
+    elif output_format == OutputFormat.json:


Testing comment at line 118 with content: return MarkDownFormatter(), print

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+    elif output_format == OutputFormat.markdown:
+        return MarkDownFormatter(), print
+    elif output_format == OutputFormat.json:
+        return JsonFormatter(), print


Testing comment at line 119 with content: elif output_format == OutputFormat.json:

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+        return MarkDownFormatter(), print
+    elif output_format == OutputFormat.json:
+        return JsonFormatter(), print
+    else:


Testing comment at line 120 with content: return JsonFormatter(), print

scastlara · 2025-05-29T11:31:03Z

src/lgtm_ai/__main__.py

+    elif output_format == OutputFormat.json:
+        return JsonFormatter(), print
+    else:
+        assert_never(output_format)


Testing comment at line 121 with content: else:

scastlara

TESTING MULTIPLES

scastlara · 2025-05-29T11:40:35Z

src/lgtm_ai/__main__.py

 import logging
 from collections.abc import Callable
 from importlib.metadata import version
-from typing import get_args


scastlara

TESTING MULTIPLES

scastlara · 2025-05-29T11:41:24Z

src/lgtm_ai/__main__.py

+    elif output_format == OutputFormat.json:
+        return JsonFormatter(), print
+    else:
+        assert_never(output_format)


TESTING 121

scastlara

TESTING MULTIPLES

scastlara · 2025-05-29T11:43:07Z

src/lgtm_ai/__main__.py

+        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))


TESTING 75 ASDASDASD

scastlara

🦉 lgtm Review

Score: Nitpicks 🤓

🔍 Summary

This PR introduces a new JSON output format, standardizes models to Pydantic, and separates log/output streams. The overall quality is high, with good structure and test coverage for the new features. The refactoring of models to Pydantic and renaming of formatters has been done consistently.

One minor improvement would be to enhance test coverage for the new JsonFormatter by adding tests for a couple of its methods.

More information

Id: fc3e22ad52374b199986e3ce17adeef5
Model: gemini-2.5-pro-preview-05-06
Created at: 2025-05-29T12:53:09.894861+00:00

Usage summary

Call 1

Request count: 1
Request tokens: 48750
Response tokens: 268
Total tokens: 104812

Call 2

Request count: 1
Request tokens: 20536
Response tokens: 703
Total tokens: 295821

Total tokens: 400633

See the 📚 lgtm-ai repository for more information about lgtm.

scastlara · 2025-05-29T12:53:10Z

tests/formatters/test_json.py

+from tests.review.utils import MOCK_DIFF, MOCK_USAGE
+
+
+class TestJsonFormatter:


🦉 🧪 Testing

Severity: LOW 🔵

class TestJsonFormatter:

Consider adding tests for format_review_comments_section to ensure it returns an empty string as designed, and for format_review_comment to verify the JSON output of a single comment. This would improve test coverage for the JsonFormatter.

Here are some examples of how you could implement these tests:

from unittest import mock # Assuming mock is used, or adjust as needed from lgtm_ai.ai.schemas import ReviewComment # Adjust import if necessary # Inside TestJsonFormatter class: def test_format_review_comments_section_returns_empty_string(self) -> None: # Test that format_review_comments_section returns an empty string as it's a no-op assert self.formatter.format_review_comments_section([]) == "" # Optionally, test with mock comments mock_comment_instance = mock.Mock(spec=ReviewComment) assert self.formatter.format_review_comments_section([mock_comment_instance]) == "" def test_format_review_comment_returns_correct_json(self) -> None: # Test that format_review_comment returns the expected JSON for a single comment test_comment_data = { "category": "Quality", "comment": "Test comment text", "is_comment_on_new_path": True, "line_number": 1, "new_path": "test.py", "old_path": "test.py", "programming_language": "python", "relative_line_number": 1, "severity": "LOW", "quote_snippet": "foo bar" } # Ensure ReviewComment can be instantiated this way, or use a factory/mock review_comment = ReviewComment(**test_comment_data) expected_json = review_comment.model_dump_json(indent=2) assert self.formatter.format_review_comment(review_comment) == expected_json

More information about this comment

File: tests/formatters/test_json.py

Line: 19

Relative line: 19

scastlara

🦉 lgtm Review

Score: Nitpicks 🤓

🔍 Summary

This PR introduces valuable enhancements, notably the JSON output format, a new --output-format CLI option, the consistent adoption of Pydantic models, and improved log/output stream separation. The overall quality is good.

Key areas for attention:

Testing: The tests for JsonFormatter in tests/formatters/test_json.py should use actual PublishMetadata instances instead of mocks to ensure accurate testing of uuid and created_at field serialization. This affects tests around line 24 and line 117.
Code Quality:
- In src/lgtm_ai/formatters/json.py (line 6), the format_review_summary_section method has an unused comments parameter. Consider removing it or marking it as intentionally unused for clarity.
- In src/lgtm_ai/__main__.py (around line 158), the call to format_review_comments_section is redundant for the JSON format and could be conditionally skipped.

Addressing these points will further improve the robustness and cleanliness of the introduced features.

More information

Id: 3aa9905b7c8943579a9f36965d236db8
Model: gemini-2.5-pro-preview-05-06
Created at: 2025-05-29T16:45:19.299413+00:00

Usage summary

Call 1

Request count: 1
Request tokens: 52572
Response tokens: 1684
Total tokens: 297367

Call 2

Request count: 1
Request tokens: 21962
Response tokens: 1289
Total tokens: 397435

Total tokens: 694802

See the 📚 lgtm-ai repository for more information about lgtm.

scastlara · 2025-05-29T16:45:20Z

tests/formatters/test_json.py

+
+    def test_format_summary_section(self) -> None:
+        review = Review(
+            metadata=mock.Mock(


🦉 🧪 Testing

Severity: MEDIUM 🟡

metadata=mock.Mock( uuid="fb64cb958fcf49219545912156e0a4a0", model_name="whatever", created_at="2025-05-15T09:43:01.654374+00:00", usages=[MOCK_USAGE] * 3, spec=PublishMetadata, ),

In both test_format_summary_section (around line 24) and test_format_guide (around line 117), the metadata attribute (which is a PublishMetadata object) is mocked using mock.Mock(...). The JSON assertions for this "metadata" part in the tests currently only expect model_name and usages, and do not include uuid and created_at.
However, PublishMetadata defines uuid and created_at as @cached_property fields, which Pydantic V2 typically includes in serialization.
Using mock.Mock here might not accurately test how a real PublishMetadata instance serializes these fields.
Consider instantiating an actual PublishMetadata object for metadata in these tests to ensure accurate verification of the serialization behavior, including uuid and created_at if they are expected in the JSON output. If they are not expected, the reason should be clear from PublishMetadata's design or the test's intent.

More information about this comment

File: tests/formatters/test_json.py

Line: 24

Relative line: 24

scastlara · 2025-05-29T16:45:20Z

src/lgtm_ai/formatters/json.py

+
+
+class JsonFormatter(Formatter[str]):
+    def format_review_summary_section(self, review: Review, comments: list[ReviewComment] | None = None) -> str:


🦉 ✨ Quality

Severity: LOW 🔵

def format_review_summary_section(self, review: Review, comments: list[ReviewComment] | None = None) -> str:

The comments parameter in format_review_summary_section is declared but not used. In other formatters, this parameter can be used for details about comments that failed to publish. For the JSON formatter, since the entire review object is serialized, this parameter might be redundant or misleading.
Consider prefixing it with an underscore (e.g., _comments) if it's intentionally unused due to interface conformity, or remove it if the Formatter base class or interface allows for this variation.

More information about this comment

File: src/lgtm_ai/formatters/json.py

Line: 6

Relative line: 6

scastlara · 2025-05-29T16:45:20Z

src/lgtm_ai/__main__.py

+        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))


🦉 ✨ Quality

Severity: LOW 🔵

printer(formatter.format_review_summary_section(review)) if review.review_response.comments: printer(formatter.format_review_comments_section(review.review_response.comments))

For the JSON output format, formatter.format_review_summary_section(review) (line 156) already outputs the entire review, including all comments. Therefore, the subsequent call to formatter.format_review_comments_section(...) on line 158 becomes redundant when output_format is JSON, as JsonFormatter.format_review_comments_section returns an empty string.
To streamline this, consider making the call on line 158 conditional. For example:

printer(formatter.format_review_summary_section(review)) if resolved_config.output_format != OutputFormat.json and review.review_response.comments: printer(formatter.format_review_comments_section(review.review_response.comments))

More information about this comment

File: src/lgtm_ai/__main__.py

Line: 158

Relative line: 75

scastlara

🦉 lgtm Review

Score: Needs Work 🔧

🔍 Summary

The PR introduces a new JSON output formatter and integrates it seamlessly via a --output-format CLI option and config file support. The refactoring to use Pydantic models across the board is a significant improvement, enhancing data validation and clarity. The separation of logs to stderr for RichHandler is also a good practice.

However, a critical issue has been identified in the newly added JSON formatter tests. The tests for JsonFormatter rely on mocked PublishMetadata objects that include dynamically generated uuid and created_at values. The current assertions in these tests use hardcoded expected values for these dynamic fields, which will lead to test failures or flakiness. This needs to be addressed to ensure the tests accurately reflect the intended behavior and are deterministic.

Overall, this is a well-structured PR with significant improvements to the codebase, but the identified test issue needs to be resolved before merging.

Changes Required:

Fix tests/formatters/test_json.py: Correct the tests that assert against dynamically generated uuid and created_at fields by instantiating PublishMetadata with fixed, predictable values.

More information

Id: e73b9f54b5cb4a169be2b610b9c1f07e
Model: gemini-2.5-flash-preview-05-20
Created at: 2025-05-29T16:50:41.402961+00:00

Usage summary

Call 1

Request count: 1
Request tokens: 52572
Response tokens: 874
Total tokens: 56418

Call 2

Request count: 1
Request tokens: 21151
Response tokens: 1119
Total tokens: 95356

Total tokens: 151774

See the 📚 lgtm-ai repository for more information about lgtm.

scastlara · 2025-05-29T16:50:42Z

tests/formatters/test_json.py

+
+    def test_format_summary_section(self) -> None:
+        review = Review(
+            metadata=mock.Mock(


🦉 ✅ Correctness

Severity: HIGH 🔴

review = Review( metadata=mock.Mock( uuid="fb64cb958fcf49219545912156e0a4a0", model_name="whatever", created_at="2025-05-15T09:43:01.654374+00:00", usages=[MOCK_USAGE] * 3, spec=PublishMetadata, ), review_response=ReviewResponse( raw_score=5, summary="summary", ), pr_diff=PRDiff( id=1, diff=MOCK_DIFF, changed_files=["file1", "file2"], target_branch="main", source_branch="feature" ), ) # ... (rest of the test) def test_format_guide(self) -> None: guide = ReviewGuide( pr_diff=PRDiff( id=1, diff=MOCK_DIFF, changed_files=["file1", "file2"], target_branch="main", source_branch="feature" ), guide_response=GuideResponse( summary="summary", key_changes=[ GuideKeyChange( file_name="foo.py", description="description", ), GuideKeyChange( file_name="bar.py", description="description", ), ], checklist=[GuideChecklistItem(description="item 1")], references=[ GuideReference(title="title", url="https://example.com"), ], ), metadata=mock.Mock( uuid="fb64cb958fcf49219545912156e0a4a0", model_name="whatever", created_at="2025-05-15T09:43:01.654374+00:00", usages=[MOCK_USAGE], spec=PublishMetadata, ), ) # ... (rest of the test)

The uuid and created_at fields of PublishMetadata generate dynamic values. Mocking metadata with mock.Mock while asserting against hardcoded uuid and created_at values in the expected JSON will cause these tests to fail or be flaky. Instead of mocking the entire metadata object, you should instantiate PublishMetadata directly with fixed, deterministic values for uuid (as a string) and created_at (as a datetime object) to ensure test stability and reliability. This applies to both test_format_review_summary_section and test_format_guide.

import datetime import uuid # ... (other imports and code) class TestJsonFormatter: formatter = JsonFormatter() def test_format_review_summary_section(self) -> None: fixed_uuid_str = <details><summary>More information about this comment</summary> - **File**: `tests/formatters/test_json.py` - **Line**: `24` - **Relative line**: `24` </details>

feat(#18): add json formatter and new --output-format option and co…

1aa92b3

…nfig

scastlara requested review from jbozanowski, sacha-c and sdn4z as code owners May 29, 2025 09:42

github-actions bot added the feature label May 29, 2025

github-actions bot reviewed May 29, 2025

View reviewed changes

scastlara mentioned this pull request May 29, 2025

Comment placement on GitHub is sometimes wrong #32

Closed

sdn4z approved these changes May 29, 2025

View reviewed changes

scastlara commented May 29, 2025

View reviewed changes

elementsinteractive deleted a comment from github-actions bot May 29, 2025

scastlara mentioned this pull request May 29, 2025

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

Merged

scastlara commented May 29, 2025

View reviewed changes

scastlara merged commit 95777fb into main May 29, 2025
7 checks passed

github-actions bot added feature and removed feature labels May 29, 2025

scastlara commented May 29, 2025

View reviewed changes

		logger.setLevel(logging.DEBUG)
		logger.info("Logging level set to %s", logging.getLevelName(logger.level))

		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]]:



		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."""

		from tests.review.utils import MOCK_DIFF, MOCK_USAGE


		class TestJsonFormatter:



		class JsonFormatter(Formatter[str]):
		def format_review_summary_section(self, review: Review, comments: list[ReviewComment] \| None = None) -> str:

Conversation

scastlara commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scastlara commented May 29, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

🦉 lgtm Review

🔍 Summary

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scastlara commented May 29, 2025 •

edited

Loading