Skip to content

fix: handle comma thousands separators in inlay hint amount parsing#839

Open
vqv wants to merge 1 commit intopolarmutex:mainfrom
vqv:fix-comma-thousands-separator-inlay-hints
Open

fix: handle comma thousands separators in inlay hint amount parsing#839
vqv wants to merge 1 commit intopolarmutex:mainfrom
vqv:fix-comma-thousands-separator-inlay-hints

Conversation

@vqv
Copy link
Copy Markdown

@vqv vqv commented Mar 30, 2026

Summary

Fixes a bug where inlay hints showed spurious implicit postings on balanced transactions when one leg used comma thousands separators and the other didn't.

For example, this balanced transaction incorrectly showed an implicit posting hint:

2024-01-01 * "Credit Card Payment"
  Liabilities:CreditCard    19,987.73 USD
  Assets:Checking           -19987.73 USD

Root cause: Decimal::from_str_exact() does not accept commas, so amounts like 19,987.73 failed to parse and were treated as None (missing amount) — triggering the implicit posting hint.

Fix: Strip comma thousands separators from number strings before passing to Decimal::from_str_exact() in both extract_amount_from_node() and extract_compound_amount(). This matches beancount's own behavior, which only supports commas as thousands separators (not European-style period separators).

Related to #750

Test plan

  • Added test_comma_thousands_separator_balanced — verifies no spurious hint on balanced transactions with comma-formatted amounts
  • Added test_comma_thousands_separator_with_implicit_posting — verifies correct balancing calculation when one posting is implicit and the other uses commas
  • All 18 existing inlay hint tests continue to pass

Decimal::from_str_exact() does not accept commas, so amounts like
"19,987.73" failed to parse and were treated as missing — causing
spurious implicit-posting inlay hints on balanced transactions.

Strip commas from number strings before parsing in both
extract_amount_from_node() and extract_compound_amount().

Closes polarmutex#750 (partially — the inlay hint portion)
@polarmutex
Copy link
Copy Markdown
Owner

Thanks for this fix — the root cause analysis is correct and the test coverage looks solid. The fix to strip commas before Decimal::from_str_exact() is consistent with beancount's own parsing behavior.

The CI run shows action_required (fork workflow approval pending). I'll approve the CI run so tests can complete. Once CI is green, this is ready to merge.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 83.07692% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.89%. Comparing base (d0d7e0c) to head (141639b).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
crates/lsp/src/providers/inlay_hints.rs 83.07% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
+ Coverage   80.88%   80.89%   +0.01%     
==========================================
  Files          31       31              
  Lines       12502    12567      +65     
==========================================
+ Hits        10112    10166      +54     
- Misses       2390     2401      +11     
Files with missing lines Coverage Δ
crates/lsp/src/providers/inlay_hints.rs 86.63% <83.07%> (-0.27%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@polarmutex
Copy link
Copy Markdown
Owner

CI is nearly all green — all Rust tests pass. There is one failure in the VS Code Extension / Lint & format step:

[warn] package.json
Code style issues found in the above file. Run Prettier with --write to fix.

The vscode/package.json changes need to be formatted with Prettier. You can fix this by running in the vscode/ directory:

pnpm run format

or:

pnpm exec prettier --write package.json

Then commit and push. Once that's addressed and CI is fully green, this is ready to merge.

@vqv vqv force-pushed the fix-comma-thousands-separator-inlay-hints branch from 141639b to 660b949 Compare April 9, 2026 00:32
@vqv
Copy link
Copy Markdown
Author

vqv commented Apr 9, 2026

Removed the unrelated diagnosticFlags commit that was causing the lint failure. This PR is now focused on just the comma thousands separator fix.

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