register: respect account-specific depth with report intervals - #2740
ryanduguid wants to merge 2 commits into
Conversation
acinader
left a comment
There was a problem hiding this comment.
As part of learning how to do my own accounting, I am trying to understand why the command line arguments exist and how I could use them to better inspect and validate my own books.
This looked interesting, so, using claude, I checked this out, built a test plan ran through before and after command line invocations so I could visuallly understand what is going on here.
This looks correct to me.
Some findings:
Built the PR head (e5506bbed) and its parent (f6f7661d0), GHC 9.14.1,
mac-aarch64.
hledger test: 250/250.just functest: 1904/1904.hledger/test/register/:
64/64, including the 12 new cases.- Interval registers match
balanceoutput for every depth spelling I tried. - Performance unchanged: 1.39s (parent) vs 1.40s (head) on
register -D -E -p '1995-01-01..2035-01-01' --depth 2overexamples/1ktxns-100accts.journal
(~14,600 intervals).
1. register and register -M now disagree
$ hledger -f examples/sample2.journal register assets --depth 1 --depth checking=2 -M
2025-01 assets 1 AAA 1 AAA
1100 USD 1100 USD
assets:bank 750 USD 1 AAA
1850 USD
$ hledger -f examples/sample2.journal register assets --depth 1 --depth checking=2
2025-01-01 starting balances assets 100 USD 100 USD
assets 1000 USD 1100 USD
assets 1000 USD 2100 USD
(all 8 posting rows assets; 5 of them are assets:bank:checking postings, and
hledger accounts with the same flags lists assets:bank.)
Cause: Register.hs:175/:276 clip a second time, after
PostingsReport.hs:195. The second clip is not idempotent when the rule's
regexp matched a component the first clip removed — checking does not match
assets:bank, so the flat depth 1 applies. Pre-existing: reproduces on 1.99.4
and on f6f7661d0. The isJust mperiod guards exempt summary rows from it.
Replacing both guards with clipAcct = id — no display-time clipping, since
postingsReportItems and summarisePostingsInDateSpan have both already
clipped — makes the two agree, leaves this PR's interval output unchanged, and
passes just functest 1904/1904. Built and run. Caveat: postingsReportItems
clips with queryDepth (_rsQuery rspec), Register.hs with
depth_ (_rsReportOpts rspec); the former is the wider spec.
2. Three changes vs f6f7661d0, untested and not in the commit message
- Interval rows sort by clipped name (
M.toAscList), not full name. With
expenses1:aaandexpenses:bb,register -M --depth 1printedexpenses1
first, now printsexpensesfirst, asbalance -M --depth 1does. Running
total follows the new order. Rows reorder throughout on
examples/1ktxns-100accts.journal. Only diff a 40-command sweep of interval
registers without account-specific depth found between parent and head. - Rows whose postings cancel to zero in an interval are dropped without
-E;
previously printed as two rows (+100,-100). - One clipped account in several commodities is one row, not one row per
commodity with separate running totals.
3. depth: query form also fixed, untested
register expenses -M --depth 1 depth:food=2 is fixed too — it reaches the spec
via queryDepth, not the --depth flags.
4. Smaller
depth-interval.testomits the# * <topic>header and# ** N.case
numbering used bydepth.testand the rest of that directory.summaryp'spaccount=clipOrEllipsifyAccountName depthSpec ""is read only by
thenull ps && showemptybranch; every other use overwrites it.summarisePostingsByInterval's exported signature changed fromMaybe Intto
DepthSpec— an hledger-lib API change.
Cover clipped-name ordering and running totals, zero-net summaries, multiple commodities and depth: queries. Follow the register test-file headings and document the user-visible aggregation and ordering changes. Set the empty summary account only in the empty-span branch. Interval helpers remain private; no exported API signature changes.
|
Thanks for the detailed before/after checks. Updated in 68c2712.
I also reproduced the ordinary-register double clipping on both the original and PR executables, with identical CSV output for the The updated source builds with |
|
FYI for my code review - AI usage: Claude Opus 5, ~180k output tokens. |
|
Heavy token usage for that (very good) review! |
|
Thank you both. @ryanduguid this is a high quality PR. If it's AI-assisted, we'll ask you to submit something else done by hand first, to satisfy our https://hledger.org/AI.html policy. |
sounds unusual. These are some old tests broken on Windows perhaps ? |
Not AI assisted, it is purely near autonomous AI. It liked your repo. Nor me or the LLM had read the first time contributor rule before submitting. I have typed every line of this comment. |
No, they're new cases rather than old ones. This PR adds Windows isn't part of that split. Hosted CI on the same head, 68c2712, runs the full functional suite on Linux at 1,905 passed and 0 failed, and that includes the new file. The Windows failures are a separate matter and I should have kept the two further apart in the description. My local run fails 265 of 2,261 cases. The unpatched build fails 273 of the same set, and the difference is exactly the new depth cases, so the 265 are identical on both and none of them come from this change. They are executable name, path and encoding mismatches in the runner, with none in register. A base against head failure set diff gives zero new failures. |
|
Hi Ryan,
This is confusing. Anyway, I'm sorry but this PR doesn't meet our policy as it stands (first-time contributor AI use, OpenAI use, lack of disclosure in commits). The policy might change some day but for now we need to follow it fairly consistently. The best thing would probably be to submit a small PR produced without AI assistance. Then redo this one without OpenAI tools. The policy is mentioned at https://hledger.org/AI.html and https://hledger.org/PULLREQUESTS.html and in the github PR template, which begins with I'm not sure what more we could do to make it visible. |
Updated my comment to say "I have typed every line of this comment". Apologies for the confusion. |
Fixes #2423.
register -M --depth 1 --depth food=2previously combined all expenses underexpenses, discarding the account-specific depth rule. Keep the full depth specification during interval aggregation so the report retainsexpenses:foodand assigns the remaining amounts toexpenses. Thedepth:REGEXP=Nquery form works too.Sum each posting under its clipped account name and apply depth rules to the original name only once. This combines commodities into one summary per clipped account and omits zero-net summaries unless
--emptyis used, matching flat-depth behaviour. Interval summaries now sort by clipped name, which can change row order and intermediate running totals even with a flat depth limit. The final sum is unchanged. These effects are documented in the changelog and covered by functional tests.The interval helpers are private: their type changes do not change the module's exported API. The pre-existing second clipping pass for ordinary, non-interval registers remains outside this fix.
Validation on Windows with GHC 9.14.1:
stack build hledger --ghc-options=-Werror --test --no-run-tests --jobs 4passed.f6f7661dpasses six and fails 14 of these cases.just unittest: 250 tests passed.just doctest: 303 examples passed.just embedtest: passed.just testsuite is not a full local pass.Linux and macOS have not been tested locally. Hosted CI is reported separately by GitHub.