Skip to content

Conversation

@jackd248
Copy link
Owner

@jackd248 jackd248 commented Oct 23, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Documentation block parsing and merging now correctly preserves multiple occurrences of the same tag (duplicates are aggregated and retained instead of overwritten).
  • Tests

    • Added tests ensuring parsing, merging, and fixes preserve duplicate documentation tags and maintain correct order and values across round-trips.

@coderabbitai
Copy link

coderabbitai bot commented Oct 23, 2025

Walkthrough

DocBlockHeaderFixer was changed to aggregate duplicate docblock annotations: the first occurrence remains a string, subsequent occurrences convert the stored value to an array and append new values. Method PHPDoc and return-type annotations were updated to reflect values of type string|array<string>.

Changes

Cohort / File(s) Change Summary
Core fixer logic
src/Rules/DocBlockHeaderFixer.php
Adjusted parseExistingAnnotations() to return `array<string, string
Tests — duplicate annotation coverage
tests/src/Rules/DocBlockHeaderFixerTest.php
Added tests validating parsing, merging, round-trip reconstruction, and preservation of duplicate annotations (e.g., multiple @implements, duplicate author/license, empty-value tags like @internal/@api). Tests exercise internal methods via reflection.

Sequence Diagram(s)

sequenceDiagram
    participant Parser as DocBlock Parser
    participant Store as Annotations Store

    rect rgb(220,240,220)
    Note over Parser,Store: Old behavior (overwrite)
    Parser->>Store: read @tag "A"
    Store->>Store: set tag="A"
    Parser->>Store: read @tag "B"
    Store->>Store: overwrite tag="B"
    end

    rect rgb(240,220,220)
    Note over Parser,Store: New behavior (aggregate)
    Parser->>Store: read @tag "A"
    Store->>Store: set tag="A" (string)
    Parser->>Store: read @tag "B"
    Store->>Store: convert to ["A","B"] (array)
    Parser->>Store: read @tag "C"
    Store->>Store: append -> ["A","B","C"]
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

A rabbit sifts tags, gentle and spry, 🐇
First stays a string, then others stack high,
No overwrites now — they gather as friends,
Row upon row where each annotation blends,
Hooray for duplicates, together they lie! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: enhance annotation parsing to support multiple values per tag" directly and accurately captures the primary change in the changeset. The core modification is that the annotation parsing logic now aggregates duplicate tags into arrays instead of overwriting them, with corresponding type annotations updated to reflect that values can be strings or arrays of strings. The title is specific, concise, and clearly communicates the main functionality enhancement to anyone reviewing the repository history. It avoids vague language and accurately represents the changes made to both the implementation and tests.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch muliple-annotations

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2463982 and 6c5abc6.

📒 Files selected for processing (1)
  • tests/src/Rules/DocBlockHeaderFixerTest.php (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/Rules/DocBlockHeaderFixerTest.php (1)
src/Rules/DocBlockHeaderFixer.php (1)
  • configure (96-99)
🔇 Additional comments (7)
tests/src/Rules/DocBlockHeaderFixerTest.php (7)

199-212: LGTM! Well-structured test for duplicate annotation parsing.

The test correctly validates that duplicate @implements annotations are aggregated into an array, with appropriate assertions for structure and values.


214-229: LGTM! Comprehensive test for multiple duplicate tags.

The test validates both duplicate tags (author with 3 occurrences) and non-duplicate tags (single license), ensuring the aggregation logic works correctly in mixed scenarios.


231-245: LGTM! Good edge case coverage for empty annotation values.

The test validates handling of duplicate valueless annotations (like @internal, @api), ensuring empty strings are properly aggregated into arrays for duplicates.


1046-1069: LGTM! Excellent integration test for round-trip preservation.

The test validates that duplicate @implements annotations, along with other annotations, are correctly preserved through a complete parse-and-rebuild cycle. This ensures the feature works end-to-end.


1071-1088: LGTM! Critical test for merge preservation behavior.

The test validates that duplicate annotations in existing DocBlocks are preserved when merging with new annotations, ensuring the preserve_existing configuration works correctly with the duplicate annotation feature.


1090-1110: LGTM! Comprehensive end-to-end integration test.

The test validates that duplicate annotations are correctly preserved through the entire fix application process, providing critical coverage for the production use case.


199-1110: Looking at the implementation and test suite, the core functionality appears well-covered. However, to definitively verify the coverage concern, I need to identify the exact 5 changed lines mentioned in the coverage report.Based on my analysis of the implementation and comprehensive test suite, the tests appear to adequately cover the critical code paths for handling duplicate annotations. The parseExistingAnnotations method's array conversion logic and the buildDocBlock method's array iteration logic should both be exercised by tests like testFullRoundTripWithDuplicateImplementsAnnotations and testMergeWithExistingDocBlockPreservesDuplicateImplements.

However, without access to the specific lines marked as uncovered in the Coveralls report, I cannot definitively confirm whether all edge cases are tested. The 40% coverage reported could indicate either: (1) coverage metrics not capturing all test executions, or (2) specific conditional branches or error handling paths not exercised.

Approve the tests and verify coverage with local tooling.

The test implementations comprehensively cover the feature requirements:

  • Duplicate annotation parsing and array conversion ✓
  • Building DocBlocks with array values ✓
  • Full roundtrip parsing-to-building workflows ✓
  • Merging and preservation of duplicates ✓
  • All structure types (class, interface, trait, enum) ✓
  • Anonymous class handling ✓

To resolve the 40% coverage concern locally, run:

./vendor/bin/phpunit --coverage-html coverage/

Then verify that lines in parseExistingAnnotations (lines 333–339) and buildDocBlock (lines 382–386) are marked as covered (green). If they appear uncovered despite the tests executing, this may indicate a code coverage tool configuration issue rather than a test gap.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coveralls
Copy link

coveralls commented Oct 23, 2025

Pull Request Test Coverage Report for Build 18747984975

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 18747547342: 0.0%
Covered Lines: 264
Relevant Lines: 264

💛 - Coveralls

@jackd248 jackd248 merged commit 49ab418 into main Oct 23, 2025
12 checks passed
@jackd248 jackd248 deleted the muliple-annotations branch October 23, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants