refactor: code quality improvements, bug fixes, and test coverage by gagip · Pull Request #5 · gagip/git-log-analysis

gagip · 2026-02-26T05:17:40Z

Summary

Refactor: Remove dead code, simplify generics with built-in types, expose public API from package root, and clean up unreachable branches
Fix: Correct git diff path parsing (a/b/ prefix), normalize project name to lowercase for case-insensitive comparison, replace os.chdir with subprocess cwd parameter
Perf: Compute commit counts at scan time to avoid loading all commits in list_projects
Test: Add comprehensive test coverage for encoding_handler, file_processor, git_diff_parser, git_monthly_log_generator, and MCP formatters (~990 lines added across 5 new test files)

Changes

File	Description
`git_log_analysis/__init__.py`	Expose `GitLogService` and `QueryParams` from package root
`git_log_analysis/git_diff_parser.py`	Fix `a/b/` path parsing and timezone handling
`git_log_analysis/mcp/service.py`	Compute commit counts at scan time
`git_log_analysis/processors/encoding_handler.py`	Remove temp file workaround
`tests/test_*.py`	New test files for major modules

Summary by CodeRabbit

New Features
- Simpler top-level access for core services and query types.
- Public project commit counts now available.
API Changes
- Removed legacy keyword-search wrapper and trimmed some public surface.
Improvements
- Case-insensitive project matching.
- Safer subprocess handling and streamlined encoding/file parsing.
Tests
- Extensive new tests for parsing, formatting, generation, and encoding.

…zone handling 1-1: replace .replace('a/', '') with startswith+slice to avoid mangling paths that contain 'a/' or 'b/' in the middle (e.g. src/a/util.py) 1-3: replace manual date_part split with parse_date_to_naive() from mcp.utils to correctly handle both positive and negative timezone offsets

1-2: data_loader compared f.project (already lowercase) against project.lower() — now both sides are lowercased so a caller passing mixed-case project names is handled correctly. query_engine applies the same fix for commit.project filtering.

…GitLogService 1-4: data_loader and query_engine properties were unreachable dead code — _data_loader and _query_engine are always assigned in __init__ so the RuntimeError guard never fires; removed both properties. 2-1: search_by_keyword duplicated search() with an extra QueryParams construction step; callers should construct QueryParams directly and call search() instead.

… in list_projects 2-2: _scan_project_files now reads each summary JSON and tallies commits during the initial directory scan, storing results in _project_commit_counts. format_projects_list signature changed from all_commits list to project_commit_counts dict. mcp_server.py and cli/projects_command.py updated accordingly — the expensive load_commits() call on the list_projects path is eliminated.

2-3: add git_log_analysis/__init__.py that re-exports GitLogService and QueryParams so callers can use 'from git_log_analysis import ...' instead of reaching into the internal mcp sub-package. CLI commands (search_command.py, projects_command.py) updated to use the new top-level import path.

Remove List, Dict, Optional from typing imports across git_diff_parser.py, data_loader.py, query_engine.py, and service.py. Use list[], dict[], str|None, and X|None syntax available since Python 3.10+. Keep typing.Any where needed.

Use parser.parse(content) directly instead of writing a temp UTF-8 file and re-reading it. Eliminates temp file creation/deletion and avoids orphaned files if the process is interrupted.

The condition 'total_lines >= MAX_TOTAL_LINES and not limit_reached' can never be true: _format_file_change sets limit_reached=True whenever the line limit is reached, so the loop always breaks before the condition outside the loop could fire. Remove the dead code.

os.chdir changes the process-wide working directory, causing unexpected side effects when the function is called as a library. Pass cwd=repo_path to subprocess.run instead, keeping the working directory change local to the subprocess call.

Change 'assert len(results) >= 0' to 'assert len(results) > 0' with a descriptive message, ensuring the keyword search test actually verifies that results are found. Also reformat multi-line assert statements for consistency.

…f_parser, git_monthly_log_generator, and mcp formatters Add 74 new tests across 5 previously untested modules to improve coverage and catch regressions in core parsing, encoding fallback, file processing, log generation, and MCP output formatting.

coderabbitai · 2026-02-26T05:17:51Z

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Free

📥 Commits

Reviewing files that changed from the base of the PR and between 786567f and be9b941.

📒 Files selected for processing (6)

git_log_analysis/git_diff_parser.py
tests/test_encoding_handler.py
tests/test_file_processor.py
tests/test_git_diff_parser.py
tests/test_git_monthly_log_generator.py
tests/test_mcp_formatters.py

📝 Walkthrough

Walkthrough

This change re-exports GitLogService and QueryParams at package root, modernizes type hints to Python 3.10+ syntax, introduces precomputed per-project commit counts, removes temp-file I/O in encoding parsing, avoids global cwd changes for git subprocesses, and adds extensive tests.

Changes

Cohort / File(s)	Summary
Package root & service `git_log_analysis/__init__.py`, `git_log_analysis/mcp/service.py`	Re-exports `GitLogService` and `QueryParams` at package root; removes `search_by_keyword`, `data_loader`, and `query_engine` properties; adds `project_commit_counts` property; updates `get_commit_changes` signature/handling.
CLI & server callers `git_log_analysis/cli/projects_command.py`, `git_log_analysis/cli/search_command.py`, `mcp_server.py`	Imports updated to use package-root exports; projects listing now consumes `service.project_commit_counts` (no full commit list); minor CLI arg formatting changes.
Data loader & formatters `git_log_analysis/mcp/data_loader.py`, `git_log_analysis/mcp/formatters.py`	Data loader computes and exposes per-project commit counts; several method/type signatures converted to union/built-in generic syntax; `format_projects_list` now accepts `project_commit_counts: dict[str,int]`.
Diff & parsing types `git_log_analysis/git_diff_parser.py`	Switched typing annotations to Python 3.10+ built-in generics and unions across public data structures and parser methods; uses `parse_date_to_naive` for dates; adjusted return/optional semantics.
Query params & engine `git_log_analysis/mcp/query_engine.py`	`QueryParams` fields use `str
Encoding & subprocess behavior `git_log_analysis/processors/encoding_handler.py`, `git_log_analysis/git_monthly_log_generator.py`	Encoding handler now parses in-memory content (no temp files); git log generator avoids `os.chdir()` and runs `subprocess.run(..., cwd=repo_path)`.
Tests added/updated `tests/test_encoding_handler.py`, `tests/test_file_processor.py`, `tests/test_git_diff_parser.py`, `tests/test_git_monthly_log_generator.py`, `tests/test_mcp_formatters.py`, `tests/test_query_engine.py`	Adds/updates comprehensive tests for encoding handling, file processor, diff parsing, monthly log generation, formatters; some query-engine asserts reformatted.
Misc `.gitignore`	Adjusts ignore patterns (adds `settings.local.json`, `*.stackdump`).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CLI
  participant Service as GitLogService
  participant DataLoader as DataLoader (disk scan)
  participant Formatter as Formatters

  CLI->>Service: request projects list
  Service->>DataLoader: read/return project_commit_counts
  DataLoader-->>Service: dict[str,int]
  Service->>Formatter: format_projects_list(projects, project_commit_counts)
  Formatter-->>CLI: formatted output

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hopped through types and tidy rows,

swapped temp files for in-memory flows,
opened root doors for quick import,
counted commits in every sort,
tests now cheer — a carrot treat for devs who write neat code. 🥕

Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This PR implements code quality improvements, bug fixes, performance optimizations, and comprehensive test coverage enhancements for the git log analysis tool. The changes migrate to Python 3.10+ type hints, remove dead code, fix path parsing bugs, improve performance, and add ~990 lines of test coverage across 5 new test files.

Changes:

Fixed git diff path parsing to correctly handle paths containing 'a/' or 'b/' segments (e.g., src/a/util.py)
Replaced os.chdir with subprocess cwd parameter for better isolation
Removed temporary file workaround in encoding handler, simplified to use parser.parse() directly
Computed commit counts at scan time to avoid loading all commits when listing projects
Removed dead/unreachable code (duplicate search method, unused properties, unreachable formatter condition)
Migrated from typing.Optional/typing.Dict to Python 3.10+ union syntax (str | None, dict[str, int])
Exposed GitLogService and QueryParams from package root for easier importing
Added extensive test coverage for parsers, formatters, and utility modules

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`git_log_analysis/__init__.py`	New file exposing public API from package root
`git_log_analysis/cli/search_command.py`	Updated import to use new public API and code formatting
`git_log_analysis/cli/projects_command.py`	Updated to use pre-computed commit counts instead of loading all commits
`git_log_analysis/git_diff_parser.py`	Fixed path parsing bug (prefix stripping), improved timezone handling, migrated to modern type hints
`git_log_analysis/git_monthly_log_generator.py`	Replaced `os.chdir` with subprocess `cwd` parameter
`git_log_analysis/mcp/data_loader.py`	Compute commit counts at scan time, improved case-insensitive project filtering, type hint migration
`git_log_analysis/mcp/formatters.py`	Changed `format_projects_list` signature to accept pre-computed counts, removed unreachable code, formatting improvements
`git_log_analysis/mcp/query_engine.py`	Case-insensitive project comparison, type hint migration
`git_log_analysis/mcp/service.py`	Removed dead code (unused methods/properties), exposed `project_commit_counts`, type hint migration
`git_log_analysis/processors/encoding_handler.py`	Removed temporary file workaround, simplified to use `parser.parse()` directly
`mcp_server.py`	Updated to use pre-computed `project_commit_counts` instead of loading all commits
`tests/test_encoding_handler.py`	New comprehensive tests for encoding fallback functionality
`tests/test_file_processor.py`	New tests for file processor save and parse operations
`tests/test_git_diff_parser.py`	New comprehensive tests for diff and log parsing, including path edge cases
`tests/test_git_monthly_log_generator.py`	New tests for month range generation and log generation with subprocess validation
`tests/test_mcp_formatters.py`	New comprehensive tests for all formatter functions
`tests/test_query_engine.py`	Improved assertion messages for better test failure diagnostics

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…check

gagip added 11 commits February 26, 2026 13:54

refactor: remove temp file workaround in encoding_handler

5b579aa

Use parser.parse(content) directly instead of writing a temp UTF-8 file and re-reading it. Eliminates temp file creation/deletion and avoids orphaned files if the process is interrupted.

test: replace always-true assert with meaningful assertion

3a3db07

Change 'assert len(results) >= 0' to 'assert len(results) > 0' with a descriptive message, ensuring the keyword search test actually verifies that results are found. Also reformat multi-line assert statements for consistency.

gagip requested a review from Copilot February 26, 2026 05:18

Copilot started reviewing on behalf of gagip February 26, 2026 05:18 View session

chore: Add stackdump in gitignore

786567f

Copilot AI reviewed Feb 26, 2026

View reviewed changes

fix: remove unused imports and ambiguous variable names to pass ruff …

be9b941

…check

gagip merged commit 6dab667 into master Feb 26, 2026
4 checks passed

gagip deleted the refactor branch February 26, 2026 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: code quality improvements, bug fixes, and test coverage#5

refactor: code quality improvements, bug fixes, and test coverage#5
gagip merged 13 commits intomasterfrom
refactor

gagip commented Feb 26, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Feb 26, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gagip commented Feb 26, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gagip commented Feb 26, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 26, 2026 •

edited

Loading