AmountFormatExt: central place to explain why we use the somewhat redundant allCommodities parameter

[thielema]

The type highlights the connection between allCommodities and AmountFormat.

multiBalanceRowAsCsvText: swap order of parameters since allCommodities plays the role of AmountFormat here.

This is my proposal to address this objection in a non-invasive way:
#2728 (comment)

Since allCommodities is only needed for LayoutBareWide we could add an according parameter to the LayoutBareWide constructor. But then we would likely need a type parameter for Layout for the LayoutBareWide-associated data since we do not always want to carry a [CommoditySymbol].

We could split AmountFormat into a smaller type for report generating functions and a larger type for showMixedAmountWith & Co. The larger type could carry the smaller one in a record field.

Both are pretty invasive changes, though.

[simonmichael]

I'm not too keen on this. I think just a comment would give more bang for the buck.

[thielema]

On Sun, 13 Sep 2026, Simon Michael wrote: [10144?s=20&v=4] simonmichael left a comment (hledgerorg/hledger#2730) I'm not too keen on this. I think just a comment would give more bang for the buck.

A simple comment isn't as easy as it looks first. I need to choose a place where to attach the comment such that it is found when someone wonders. There are two functions that are candidates for the comment, but maybe even the callers are the better place. And the other places have to refer to that comment in order to not get missed, right? I think the Clean Coders are right here: Better try to make code clearer by primary language features. Thus I am quite happy with the type synonym solution.

[simonmichael]

Agreed, but I'm not convinced this makes things clearer.

Mine is only a gut feeling, but here are some more specifics from ai:

fable> Here's my read of it. Context: this is thielema's follow-up to your comment on #2728, where you declined the "put allCommodities in AmountFormat's displayCommodityOrder" change but suggested at least documenting the footgun in a dev comment. This PR responds by introducing type AmountFormatExt = (AmountFormat, [CommoditySymbol]) as a home for that documentation, and reordering parameters across ~6 functions so the commodity list travels next to (or in place of) the format argument. It's purely mechanical — no behavior change, and the alternatives the body mentions (a field on LayoutBareWide, splitting AmountFormat) are correctly identified as more invasive.

My assessment: it delivers more churn and less documentation than what you actually asked for.

The type synonym adds no safety. AmountFormatExt is just a name for a tuple — the compiler accepts any (AmountFormat, [CommoditySymbol]) pair, including a wrong or empty list, so the unenforced invariant is exactly as unenforced as before. Its real function is to be a place to hang a haddock comment, which a plain comment on the existing parameters would accomplish with a near-zero diff.

The new comment omits the actual footgun. The haddock says the symbols are only used for LayoutBareWide and override displayCommodityOrder, but not the dangerous part you quoted in #2728: the list must be a superset of every rendered commodity, because orderedAmounts silently drops missing ones and prints zeros. That's the sentence future maintainers need.

The abstraction is applied inconsistently. Functions that already took an AmountFormat get the tuple; functions that don't (multiBalanceRowAsText, multiBalanceReportAsPartTable) instead get their [CommoditySymbol] moved before ReportOpts to hint at the association. That convention is invisible to a reader, and the reordering churns every call site (including CompoundBalanceCommand.hs) for a mnemonic. Tuple-typed arguments with fmtExt@(_fmt, allCommodities) pattern matches are also a bit against the grain of this codebase's curried style.

It does keep AmountFormatExt private-ish (defined in Balance.hs, only shared with CompoundBalanceCommand), which is the right instinct if you did want the type.

[thielema]

On Sun, 13 Sep 2026, Simon Michael wrote: Agreed, but I'm not convinced this makes things clearer. Mine is only a gut feeling, but here are some more specifics from ai: The type synonym adds no safety. AmountFormatExt is just a name for a tuple — the compiler accepts any (AmountFormat, [CommoditySymbol]) pair, including a wrong or empty list, so the unenforced invariant is exactly as unenforced as before.

The invariant is unenforced and I am afraid Haskell's type system is not strong enough to enforce that invariant. Liquid Haskell could do these kind of things, but for my judgement is not yet production ready. However, giving a list with missing commodity symbols can be even used as feature, namely as a commodity filter. That said, the problem with the unenforced invariant arises only for the report builders were I export partial builders for re-use in CompoundBalanceCommand. In the complete builders, allCommodities is just a local variable. I offer to move those partial builders and other helper functions to a private module, say BalancePrivate.hs. This is a good idea anyway to remove distraction for users of the library interface.

[simonmichael]

Sorry, I don't think the tuple adds clarity here. I have pushed some comments instead.

If you think the private module is a win, let's try that as a new PR. I think typical haskell naming would be .Internal ? I'm not quite seeing the benefit of complicating our module tree, given that we can control visibility with exports (and we don't have many library users).

[thielema]

On Mon, 21 Sep 2026, Simon Michael wrote: [10144?s=20&v=4] simonmichael left a comment (hledgerorg/hledger#2730) Sorry, I don't think the tuple adds clarity here. I have pushed some comments instead. If you think the private module is a win, let's try that as a new PR. I think typical haskell naming would be .Internal ? I'm not quite seeing the benefit of complicating our module tree, given that we can control visibility with exports (and we don't have many library users).

We have some functions, that shall be used by functions in Cli.Commands.Balance and CompoundBalanceCommand but shall not be externally visible. How would you solve this without an additional private module?

[simonmichael]

I see. I could also just export from Balance with special naming and haddock, as a special case. But I see your point.