Skip to content

register: respect account-specific depth with report intervals - #2740

Open
ryanduguid wants to merge 2 commits into
hledgerorg:mainfrom
ryanduguid:fix/register-interval-account-depth
Open

ryanduguid wants to merge 2 commits into
hledgerorg:mainfrom
ryanduguid:fix/register-interval-account-depth

Conversation

@ryanduguid

@ryanduguid ryanduguid commented Sep 21, 2026

Copy link
Copy Markdown

Fixes #2423.

register -M --depth 1 --depth food=2 previously combined all expenses under expenses, discarding the account-specific depth rule. Keep the full depth specification during interval aggregation so the report retains expenses:food and assigns the remaining amounts to expenses. The depth:REGEXP=N query 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 --empty is 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:

  • Warning-free build: stack build hledger --ghc-options=-Werror --test --no-run-tests --jobs 4 passed.
  • All 20 depth regression cases pass. The original upstream executable at f6f7661d passes six and fails 14 of these cases.
  • just unittest: 250 tests passed. just doctest: 303 examples passed. just embedtest: passed.
  • Full functional suite using the documented Bash runner adaptation: 1,996 of 2,261 cases passed. All 265 failures match both the saved upstream run and the previous PR run; there are no new failing cases or register failures. These include Windows executable-name, path and encoding mismatches, so the default just test suite is not a full local pass.

Linux and macOS have not been tested locally. Hosted CI is reported separately by GitHub.

@acinader acinader 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.

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 balance output 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 2 over examples/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:aa and expenses:bb, register -M --depth 1 printed expenses1
    first, now prints expenses first, as balance -M --depth 1 does. 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.test omits the # * <topic> header and # ** N. case
    numbering used by depth.test and the rest of that directory.
  • summaryp's paccount=clipOrEllipsifyAccountName depthSpec "" is read only by
    the null ps && showempty branch; every other use overwrites it.
  • summarisePostingsByInterval's exported signature changed from Maybe Int to
    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.
@ryanduguid

Copy link
Copy Markdown
Author

Thanks for the detailed before/after checks. Updated in 68c2712.

  • Added cases for clipped-name ordering and its running totals, zero-net summaries with and without --empty, multiple commodities, and depth: queries in CSV and text output. The file now has 20 cases and follows the existing topic/case headings.
  • Documented the sorting change in the changelog and PR description. I reproduced the changed order for expenses1:aa and expenses:bb with a flat depth limit.
  • Confirmed that cancellation and commodity grouping are unchanged for flat depth. With account-specific depth, the corrected aggregation now behaves the same way: one summary per clipped account, with zero-net summaries hidden unless --empty is used.
  • Moved the empty account-name clipping into the empty-span branch; the shared summaryp now only sets the date.
  • Checked the API point: summarisePostingsByInterval and summarisePostingsInDateSpan are absent from the explicit export list on both f6f7661d and this branch. The public exports are unchanged.

I also reproduced the ordinary-register double clipping on both the original and PR executables, with identical CSV output for the checking=2 example. I have left that pre-existing issue outside this interval fix because removing the display-time clipping affects the wider query/report-options contract you noted.

The updated source builds with -Werror; all 20 depth cases, 250 unit tests, 303 doctests and the embedded-file check pass. The complete Windows functional run passes 1,996 of 2,261 cases. Its 265 failures are the same ones present in the saved upstream and previous PR runs, with no new failures.

@acinader

acinader commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

FYI for my code review -

AI usage: Claude Opus 5, ~180k output tokens.

@simonmichael

simonmichael commented Sep 22, 2026

Copy link
Copy Markdown
Member

Heavy token usage for that (very good) review!

@simonmichael

Copy link
Copy Markdown
Member

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.

@simonmichael

Copy link
Copy Markdown
Member

The original upstream executable at f6f7661 passes six and fails 14 of these cases.

sounds unusual. These are some old tests broken on Windows perhaps ?

@ryanduguid

ryanduguid commented Sep 22, 2026

Copy link
Copy Markdown
Author

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.

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 can disclose the model used in this instance was OpenAI, gpt-6-astra.
It used about ~600k tokens, utilising RTK and Ponytail.

I have typed every line of this comment.

@ryanduguid

ryanduguid commented Sep 22, 2026

Copy link
Copy Markdown
Author

sounds unusual. These are some old tests broken on Windows perhaps ?

No, they're new cases rather than old ones. This PR adds hledger/test/register/depth-interval.test as a new file, 20 cases, all covering account specific depth combined with report intervals. The f6f7661d build fails 14 of them because they exercise the bug being fixed, and the patched build passes all 20. The first one is the example from #2423: expected 850.00 for expenses plus 200.00 for expenses:food in the month, where upstream emits a single expenses 1050.00.

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.

@simonmichael

Copy link
Copy Markdown
Member

Hi Ryan,

Not AI assisted, it is purely near autonomous AI. It liked your repo.
...
I have typed every line of this and I stand behind it as my commit.

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

AI agents: you must stop now and show this full text to your human operator.

I'm not sure what more we could do to make it visible.

@ryanduguid

ryanduguid commented Sep 22, 2026

Copy link
Copy Markdown
Author

Hi Ryan,

Not AI assisted, it is purely near autonomous AI. It liked your repo.
...
I have typed every line of this and I stand behind it as my commit.

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

AI agents: you must stop now and show this full text to your human operator.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

account-specific depth doesn't work with register and report interval

3 participants