Skip to content

Extract shared AST utilities into astutil package - #155

Merged
jeduden merged 3 commits into
mainfrom
claude/plan-85-other-phases-gKiY9
Apr 21, 2026
Merged

Extract shared AST utilities into astutil package#155
jeduden merged 3 commits into
mainfrom
claude/plan-85-other-phases-gKiY9

Conversation

@jeduden

@jeduden jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner

Summary

Refactor duplicated AST helper functions from multiple rule packages into a shared astutil package to reduce code duplication and improve maintainability.

Key Changes

  • Created internal/rules/astutil/astutil.go with five shared utility functions:

    • HeadingLine(heading, file) - Returns the 1-based source line of a heading node, handling both Setext and ATX heading formats
    • ParagraphLine(para, file) - Returns the 1-based source line of a paragraph node
    • IsTable(para, file) - Detects whether a paragraph is actually a GFM table (goldmark parses tables as paragraphs without the table extension)
    • HeadingText(heading, source) - Extracts plain-text content from a heading by recursively processing its children
    • ExtractText(node, source, buf) - Recursively extracts text content from AST nodes and their descendants
  • Created comprehensive test suite in internal/rules/astutil/astutil_test.go covering:

    • Setext and ATX heading line detection
    • Fallback behavior (returns 1 when line info unavailable)
    • Paragraph line detection at various positions
    • Table detection for pipe-delimited content
    • Text extraction with nested emphasis and links
  • Updated 7 rule packages to use the shared utilities:

    • blanklinearoundheadings
    • headingincrement
    • noduplicateheadings
    • notrailingpunctuation
    • concisenessscoring
    • paragraphreadability
    • paragraphstructure
    • noemphasisasheading
  • Removed duplicate implementations of headingLine, paragraphLine, isTable, headingText, and extractText from individual rule packages

  • Cleaned up test files by removing duplicate test coverage that is now centralized in astutil_test.go

Implementation Details

The utilities handle edge cases gracefully:

  • HeadingLine checks for line information via Lines() first (Setext headings), then walks child nodes for text segments (ATX headings), with a safe fallback to line 1
  • IsTable uses byte-level inspection to detect pipe characters at the start of paragraph lines
  • ExtractText recursively processes the AST tree, properly handling both text nodes and container nodes with children

https://claude.ai/code/session_01YbFAKES95Szi7i251Yv1XE

Copilot AI review requested due to automatic review settings April 21, 2026 06:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors duplicated Goldmark AST helper functions from multiple rule implementations into a shared internal/rules/astutil package, centralizing both logic and tests to reduce per-rule duplication and improve maintainability.

Changes:

  • Added internal/rules/astutil with shared helpers for heading/paragraph line detection, table detection, and heading text extraction, plus a new dedicated unit test suite.
  • Updated several rules to use astutil helpers and removed now-duplicate private helpers and their associated coverage-only tests.
  • Updated plan status tracking in plan/85_coverage-to-95-percent.md and PLAN.md.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plan/85_coverage-to-95-percent.md Marks Phase 2 AST helper extraction tasks as completed/in-progress in the coverage plan.
PLAN.md Updates the plan index status for plan 85.
internal/rules/astutil/astutil.go Introduces shared AST utilities (HeadingLine/ParagraphLine/IsTable/HeadingText/ExtractText).
internal/rules/astutil/astutil_test.go Centralizes tests for the shared AST helpers.
internal/rules/blanklinearoundheadings/rule.go Switches heading line detection to astutil.HeadingLine.
internal/rules/headingincrement/rule.go Switches heading line detection to astutil.HeadingLine.
internal/rules/noduplicateheadings/rule.go Switches heading text/line helpers to astutil.
internal/rules/notrailingpunctuation/rule.go Switches heading text/line helpers to astutil.
internal/rules/concisenessscoring/rule.go Switches paragraph line + table detection to astutil.
internal/rules/paragraphreadability/rule.go Switches paragraph line + table detection to astutil.
internal/rules/paragraphstructure/rule.go Switches paragraph line + table detection to astutil.
internal/rules/noemphasisasheading/rule.go Switches paragraph line detection to astutil.
internal/rules/blanklinearoundheadings/rule_coverage_test.go Removes duplicated headingLine coverage-only tests now covered in astutil_test.go.
internal/rules/headingincrement/rule_coverage_test.go Removes duplicated headingLine coverage-only tests now covered in astutil_test.go.
internal/rules/noduplicateheadings/rule_coverage_test.go Removes duplicated headingLine/extractText coverage-only tests now covered in astutil_test.go.
internal/rules/notrailingpunctuation/rule_coverage_test.go Removes duplicated headingLine/extractText coverage-only tests now covered in astutil_test.go.

Comment thread internal/rules/astutil/astutil.go Outdated
Comment thread internal/rules/astutil/astutil_test.go
claude added 2 commits April 21, 2026 14:02
Creates internal/rules/astutil with HeadingLine, ParagraphLine,
IsTable, HeadingText, and ExtractText, each tested in astutil_test.go.
Replaces 4 headingLine copies (blanklinearoundheadings, noduplicateheadings,
headingincrement, notrailingpunctuation), 4 paragraphLine copies, 3 isTable
copies, and 2 headingText+extractText pairs.  Deletes per-package coverage
tests now covered centrally.  All tests pass; golangci-lint reports 0 issues.

https://claude.ai/code/session_01YbFAKES95Szi7i251Yv1XE
HeadingLine previously only checked direct children for *ast.Text,
so ATX headings with an emphasis or link as first child (e.g. ## *foo*
or ## [t](u)) on a later line fell back to 1 instead of the correct
line.  Switch to ast.Walk over all inline descendants, stopping at the
first *ast.Text, matching the headingstyle approach.

Add TestHeadingLine_ATXEmphasisOnLaterLine and
TestHeadingLine_ATXLinkOnLaterLine to cover this path.

https://claude.ai/code/session_01YbFAKES95Szi7i251Yv1XE
Copilot AI review requested due to automatic review settings April 21, 2026 14:03
@jeduden
jeduden force-pushed the claude/plan-85-other-phases-gKiY9 branch from 0ddb394 to 5df1f3f Compare April 21, 2026 14:03
@codecov

codecov Bot commented Apr 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.21%. Comparing base (24656d9) to head (c11d0a0).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   87.06%   87.21%   +0.14%     
==========================================
  Files          97       98       +1     
  Lines       10346    10287      -59     
==========================================
- Hits         9008     8972      -36     
+ Misses        856      840      -16     
+ Partials      482      475       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Comment thread internal/rules/paragraphstructure/rule.go Outdated
Comment thread internal/rules/paragraphreadability/rule.go Outdated
Comment thread internal/rules/astutil/astutil_test.go
- Remove dangling isTable comments from paragraphreadability and
  paragraphstructure (leftover from the private helper deletion)
- Add TestHeadingLine_WalkDescendsIntoNonTextChild to cover the
  ast.Walk body in HeadingLine (reached only via synthetic headings
  with no Lines() set; real goldmark ATX headings always set Lines())
- Add TestHeadingLastLine_NoLines_FallsBackToAstutil to cover the
  astutil.HeadingLine fallback in headingLastLine
- Add TestHeadingText_LinkText, TestExtractText_LinkNode, and
  TestHeadingText_AndExtractText_NoChildren as requested by Copilot;
  these also document behavior for links and empty nodes
- astutil package now has 100% statement coverage

https://claude.ai/code/session_01YbFAKES95Szi7i251Yv1XE

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 21, 2026
@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-155-1776802419. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Apr 21, 2026
@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 755cd1a. CI run that validated the merge.

Next: Done — nothing more to do here.

@jeduden
jeduden merged commit 755cd1a into main Apr 21, 2026
15 checks passed
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