Conversation
This commit fixes edge cases in horizontal rule detection and improves
code documentation for the scroll synchronization system.
Changes made:
1. **Updated hrRegex pattern** (MPDocument.m:1850)
- Now allows 0-3 leading spaces per CommonMark spec
- Pattern: ^[ ]{0,3}(([-][ ]*){3,}|([*][ ]*){3,}|([_][ ]*){3,})$
- Handles edge cases: spacing variants, leading whitespace
- Maintains backward compatibility with MacDown behavior
2. **Added comprehensive documentation** (MPDocument.m:1745-1783)
- Documented horizontal rule vs setext header distinction
- Listed all edge cases with examples
- Added CommonMark compatibility notes
- Referenced issue #143 for future maintainability
3. **Added detailed inline comments** (MPDocument.m:1829-1910)
- Explained each regex pattern's purpose
- Documented previousLineHadContent flag logic
- Added comments explaining setext header detection
- Clarified why checks are performed in specific order
4. **Added 40 comprehensive tests** (MPScrollSyncTests.m:573-1075)
- Tests for all three HR types (-, *, _)
- Spacing variant tests (---, - - -, irregular spacing)
- Minimum character requirement tests (2 vs 3+ characters)
- Setext vs HR context tests (previousLineHadContent behavior)
- Leading whitespace tests (0-3 spaces allowed, 4+ = code)
- Invalid pattern tests (mixed characters, trailing text)
- Complex context tests (interaction with other elements)
- Edge case tests (tabs, emphasis, underscores)
All tests follow project conventions using XCTAssertNoThrow pattern
appropriate for headless testing without UI context.
Related to #143
Remove incorrect !isHorizontalRule check that prevented setext headers with 3+ dashes from being detected correctly. The dashRegex pattern ^([-]+)$ only matches consecutive dashes (---, not - - -), so the additional !isHorizontalRule check was unnecessary and incorrectly prevented valid setext headers like "Text\n---" from being detected. This ensures: - "Text\n---" → Detected as setext header ✓ - "\n---" → NOT detected as setext header (no prior content) ✓ - "Text\n- - -" → NOT detected as setext header (spaced, not consecutive) ✓ Identified by code review. Related to #143
Updated test counts and coverage metrics after adding 40 comprehensive tests for horizontal rule regex edge cases and setext header detection. Changes: - MPScrollSyncTests.m: 574 → 1,075 lines (501 lines added) - Test count: 28 → 68 tests (40 tests added) - Total test code: ~1,396 → ~1,897 lines - Test coverage ratio: ~22 → ~30 lines of tests per source file Updated files: - plans/test_coverage_improvement_plan.md - plans/infrastructure_evaluation.md Related to #143
Contributor
Code Coverage ReportCurrent Coverage: 41.74% Coverage Details (Summary) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes edge cases in horizontal rule detection and improves scroll synchronization reliability for MacDown 3000. The changes maintain backward compatibility while improving CommonMark compliance.
Changes Made
Updated hrRegex pattern (MPDocument.m:1850)
^[ ]{0,3}(([-][ ]*){3,}|([*][ ]*){3,}|([_][ ]*){3,})$- - -), leading whitespace\s*to[ ]*for more precise space matchingFixed setext header detection bug (MPDocument.m:1886)
!isHorizontalRulecheckText\n---^([-]+)$already ensures only consecutive dashes matchAdded comprehensive documentation (MPDocument.m:1745-1916)
Added 40 comprehensive tests (MPScrollSyncTests.m:573-1075)
Updated planning documents
Related Issue
Related to #143
Manual Testing Plan
See full testing plan in comments. Key scenarios:
Review Notes
The issue will remain open for manual verification.