Skip to content

Fix 1 stale doc comment(s) across 1 file(s)#1034

Closed
LasmarKhalifa wants to merge 1 commit into
07-10/doc-comment-audit-workflowfrom
doc-audit/07-10-doc-comment-audit-workflow
Closed

Fix 1 stale doc comment(s) across 1 file(s)#1034
LasmarKhalifa wants to merge 1 commit into
07-10/doc-comment-audit-workflowfrom
doc-audit/07-10-doc-comment-audit-workflow

Conversation

@LasmarKhalifa

Copy link
Copy Markdown
Contributor

Automated doc-comment audit on top of 07-10/doc-comment-audit-workflow (base a5f0059424).

lib/roast/cog/output.rb

  • Roast::Cog::Output::WithNumber#normalize_number_string (confidence 0.9)
    • The comment says the method removes 'currency codes', but the regex [\s$¢£€¥,_] only strips whitespace, the currency symbols $¢£€¥, commas, and underscores — no letters are stripped, so alphabetic currency codes like 'USD' or 'EUR' are not removed.
    • evidence: lib/roast/cog/output.rb:255-261 (normalize_number_string implementation, regex on line 257)

Comment thread lib/roast/cog/output.rb
end

# Normalize a number string by removing separators and currency codes and validating format
# Normalize a number string by removing separators and currency symbols and validating format

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.

Inline comment on line 256 has the same stale phrase. The PR correctly fixes the method-level doc comment ("currency codes" → "currency symbols"), but the inline comment inside the method body (# Remove common digit separators and currency codes) has the exact same inaccuracy. Both should be updated together.

@LasmarKhalifa LasmarKhalifa Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. I had originally scoped this to just doc comments, but looking back it makes sense to include inline comments too. I made a few changes to the workflow itself to allow inline comments to be reviewed. I ran the same file on it again and got this PR which now fully fixes the comments! Noramlly, it would have updated the same PR instead of creating a new one, but I changed the branch naming convention since we support inline comments too, which made the new PR appear to be a new branch.

Thanks for flagging this, I think it's the right call to have inline comments being processed in the same workflow!

@LasmarKhalifa
LasmarKhalifa force-pushed the 07-10/doc-comment-audit-workflow branch from a5f0059 to 2c10411 Compare July 14, 2026 22:19
@LasmarKhalifa
LasmarKhalifa force-pushed the doc-audit/07-10-doc-comment-audit-workflow branch from b551f4d to ed8b741 Compare July 14, 2026 22:48
@LasmarKhalifa
LasmarKhalifa force-pushed the 07-10/doc-comment-audit-workflow branch from 2c10411 to af3881b Compare July 14, 2026 22:48

LasmarKhalifa commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

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.

2 participants