journal: a new account sorting mechanism, and a bunch of sorting fixes

A bunch of account sorting changes that got intermingled.

First, account codes have been dropped. They can still be parsed and
will be ignored, for now. I don't know if anyone used them.
Instead, account display order is now controlled by the order of account
directives, if any. From the mail list:

  I'd like to drop account codes, introduced in hledger 1.9 to control
  the display order of accounts. In my experience,

  - they are tedious to maintain
  - they duplicate/compete with the natural tendency to arrange account
    directives to match your mental chart of accounts
  - they duplicate/compete with the tree structure created by account
    names

  and it gets worse if you think about using them more extensively,
  eg to classify accounts by type.

  Instead, I plan to just let the position (parse order) of account
  directives determine the display order of those declared accounts.
  Undeclared accounts will be displayed after declared accounts,
  sorted alphabetically as usual.

Second, the various account sorting modes have been implemented more
widely and more correctly. All sorting modes (alphabetically, by account
declaration, by amount) should now work correctly in almost all commands
and modes (non-tabular and tabular balance reports, tree and flat modes,
the accounts command). Sorting bugs have been fixed, eg #875.
Only the budget report (balance --budget) does not yet support sorting.

Comprehensive functional tests for sorting in the accounts and balance
commands have been added. If you are confused by some sorting behaviour,
studying these tests is recommended, as sorting gets tricky.

hledger-lib/Hledger/Data/Account.hs

@@ -46,7 +46,7 @@ instance Eq Account where
 
 nullacct = Account
   { aname = ""
-  , acode = Nothing
+  , adeclarationorder = Nothing
   , aparent = Nothing
   , asubs = []
   , anumpostings = 0
@@ -67,20 +67,23 @@ accountsFromPostings ps =
     grouped = groupSort [(paccount p,pamount p) | p <- ps]
     counted = [(aname, length amts) | (aname, amts) <- grouped]
     summed =  [(aname, sumStrict amts) | (aname, amts) <- grouped]  -- always non-empty
-    nametree = treeFromPaths $ map (expandAccountName . fst) summed
-    acctswithnames = nameTreeToAccount "root" nametree
-    acctswithnumps = mapAccounts setnumps    acctswithnames where setnumps    a = a{anumpostings=fromMaybe 0 $ lookup (aname a) counted}
+    acctstree      = accountTree "root" $ map fst summed
+    acctswithnumps = mapAccounts setnumps    acctstree      where setnumps    a = a{anumpostings=fromMaybe 0 $ lookup (aname a) counted}
     acctswithebals = mapAccounts setebalance acctswithnumps where setebalance a = a{aebalance=lookupJustDef nullmixedamt (aname a) summed}
     acctswithibals = sumAccounts acctswithebals
     acctswithparents = tieAccountParents acctswithibals
     acctsflattened = flattenAccounts acctswithparents
   in
     acctsflattened
 
--- | Convert an AccountName tree to an Account tree
-nameTreeToAccount :: AccountName -> FastTree AccountName -> Account
-nameTreeToAccount rootname (T m) =
-    nullacct{ aname=rootname, asubs=map (uncurry nameTreeToAccount) $ M.assocs m }
+-- | Convert a list of account names to a tree of Account objects, 
+-- with just the account names filled in. 
+-- A single root account with the given name is added.
+accountTree :: AccountName -> [AccountName] -> Account
+accountTree rootname as = nullacct{aname=rootname, asubs=map (uncurry accountTree') $ M.assocs m }
+  where
+    T m = treeFromPaths $ map expandAccountName as :: FastTree AccountName
+    accountTree' a (T m) = nullacct{aname=a, asubs=map (uncurry accountTree') $ M.assocs m}
 
 -- | Tie the knot so all subaccounts' parents are set correctly.
 tieAccountParents :: Account -> Account
@@ -90,10 +93,6 @@ tieAccountParents = tie Nothing
       where
         a' = a{aparent=parent, asubs=map (tie (Just a')) asubs}
 
--- | Look up an account's numeric code, if any, from the Journal and set it.
-accountSetCodeFrom :: Journal -> Account -> Account
-accountSetCodeFrom j a = a{acode=fromMaybe Nothing $ lookup (aname a) (jdeclaredaccounts j)}
-
 -- | Get this account's parent accounts, from the nearest up to the root.
 parentAccounts :: Account -> [Account]
 parentAccounts Account{aparent=Nothing} = []
@@ -189,7 +188,7 @@ filterAccounts p a
     | p a       = a : concatMap (filterAccounts p) (asubs a)
     | otherwise = concatMap (filterAccounts p) (asubs a)
 
--- | Sort each level of an account tree by inclusive amount,
+-- | Sort each group of siblings in an account tree by inclusive amount,
 -- so that the accounts with largest normal balances are listed first.  
 -- The provided normal balance sign determines whether normal balances
 -- are negative or positive, affecting the sort order. Ie,
@@ -199,24 +198,54 @@ sortAccountTreeByAmount :: NormalSign -> Account -> Account
 sortAccountTreeByAmount normalsign a
   | null $ asubs a = a
   | otherwise      = a{asubs=
-                        sortBy (maybeflip $ comparing aibalance) $
+                        sortBy (maybeflip $ comparing (normaliseMixedAmountSquashPricesForDisplay . aibalance)) $
                         map (sortAccountTreeByAmount normalsign) $ asubs a}
   where
     maybeflip | normalsign==NormallyNegative = id
               | otherwise                  = flip
 
--- | Sort each level of an account tree first by the account code
--- if any, with the empty account code sorting last, and then by
--- the account name. 
-sortAccountTreeByAccountCodeAndName :: Account -> Account
-sortAccountTreeByAccountCodeAndName a
+-- | Look up an account's declaration order, if any, from the Journal and set it.
+-- This is the relative position of its account directive 
+-- among the other account directives.
+accountSetDeclarationOrder :: Journal -> Account -> Account
+accountSetDeclarationOrder j a@Account{..} = 
+  a{adeclarationorder = findIndex (==aname) (jdeclaredaccounts j)}
+
+-- | Sort account names by the order in which they were declared in
+-- the journal, at each level of the account tree (ie within each
+-- group of siblings). Undeclared accounts are sorted last and
+-- alphabetically. 
+-- This is hledger's default sort for reports organised by account.
+-- The account list is converted to a tree temporarily, adding any
+-- missing parents; these can be kept (suitable for a tree-mode report) 
+-- or removed (suitable for a flat-mode report).
+--
+sortAccountNamesByDeclaration :: Journal -> Bool -> [AccountName] -> [AccountName]
+sortAccountNamesByDeclaration j keepparents as =
+  (if keepparents then id else filter (`elem` as)) $  -- maybe discard missing parents that were added
+  map aname $                                         -- keep just the names
+  drop 1 $                                            -- drop the root node that was added
+  flattenAccounts $                                   -- convert to an account list
+  sortAccountTreeByDeclaration $                      -- sort by declaration order (and name)
+  mapAccounts (accountSetDeclarationOrder j) $        -- add declaration order info  
+  accountTree "root"                                  -- convert to an account tree
+  as
+
+-- | Sort each group of siblings in an account tree by declaration order, then account name.
+-- So each group will contain first the declared accounts, 
+-- in the same order as their account directives were parsed, 
+-- and then the undeclared accounts, sorted by account name. 
+sortAccountTreeByDeclaration :: Account -> Account
+sortAccountTreeByDeclaration a
   | null $ asubs a = a
   | otherwise      = a{asubs=
-      sortBy (comparing accountCodeAndNameForSort) $ map sortAccountTreeByAccountCodeAndName $ asubs a}
+      sortBy (comparing accountDeclarationOrderAndName) $ 
+      map sortAccountTreeByDeclaration $ asubs a
+      }
 
-accountCodeAndNameForSort a = (acode', aname a)
+accountDeclarationOrderAndName a = (adeclarationorder', aname a)
   where
-    acode' = fromMaybe maxBound (acode a)
+    adeclarationorder' = fromMaybe maxBound (adeclarationorder a)
 
 -- | Search an account list by name.
 lookupAccount :: AccountName -> [Account] -> Maybe Account

hledger-lib/Hledger/Data/Journal.hs

@@ -256,7 +256,7 @@ journalAccountNamesImplied = expandAccountNames . journalAccountNamesUsed
 
 -- | Sorted unique account names declared by account directives in this journal.
 journalAccountNamesDeclared :: Journal -> [AccountName]
-journalAccountNamesDeclared = nub . sort . map fst . jdeclaredaccounts
+journalAccountNamesDeclared = nub . sort . jdeclaredaccounts
 
 -- | Sorted unique account names declared by account directives or posted to
 -- by transactions in this journal.
@@ -493,7 +493,8 @@ journalFinalise t path txt assrt j@Journal{jfiles=fs} =
    journalApplyCommodityStyles $
    j {jfiles        = (path,txt) : reverse fs
      ,jlastreadtime = t
-     ,jtxns         = reverse $ jtxns j -- NOTE: see addTransaction
+     ,jdeclaredaccounts = reverse $ jdeclaredaccounts j
+     ,jtxns         = reverse $ jtxns j         -- NOTE: see addTransaction
      ,jtxnmodifiers = reverse $ jtxnmodifiers j -- NOTE: see addTransactionModifier
      ,jperiodictxns = reverse $ jperiodictxns j -- NOTE: see addPeriodicTransaction
      ,jmarketprices = reverse $ jmarketprices j -- NOTE: see addMarketPrice

hledger-lib/Hledger/Data/Ledger.hs

@@ -63,7 +63,7 @@ ledgerFromJournal q j = nullledger{ljournal=j'', laccounts=as}
     (q',depthq)  = (filterQuery (not . queryIsDepth) q, filterQuery queryIsDepth q)
     j'  = filterJournalAmounts (filterQuery queryIsSym q) $ -- remove amount parts which the query's sym: terms would exclude
           filterJournalPostings q' j
-    as  = map (accountSetCodeFrom j) $ accountsFromPostings $ journalPostings j'
+    as  = accountsFromPostings $ journalPostings j'
     j'' = filterJournalPostings depthq j'
 
 -- | List a ledger's account names.

hledger-lib/Hledger/Data/Types.hs

@@ -358,11 +358,11 @@ data Journal = Journal {
   ,jparseparentaccounts   :: [AccountName]                         -- ^ the current stack of parent account names, specified by apply account directives
   ,jparsealiases          :: [AccountAlias]                        -- ^ the current account name aliases in effect, specified by alias directives (& options ?)
   -- ,jparsetransactioncount :: Integer                               -- ^ the current count of transactions parsed so far (only journal format txns, currently)
-  ,jparsetimeclockentries :: [TimeclockEntry]                   -- ^ timeclock sessions which have not been clocked out
+  ,jparsetimeclockentries :: [TimeclockEntry]                       -- ^ timeclock sessions which have not been clocked out
   -- principal data
-  ,jdeclaredaccounts      :: [(AccountName, Maybe AccountCode)]     -- ^ Accounts declared by account directives, in parse order.
+  ,jdeclaredaccounts      :: [AccountName]                          -- ^ Accounts declared by account directives, in parse order (after journal finalisation) 
   ,jcommodities           :: M.Map CommoditySymbol Commodity        -- ^ commodities and formats declared by commodity directives
-  ,jinferredcommodities   :: M.Map CommoditySymbol AmountStyle      -- ^ commodities and formats inferred from journal amounts  XXX misnamed
+  ,jinferredcommodities   :: M.Map CommoditySymbol AmountStyle      -- ^ commodities and formats inferred from journal amounts  TODO misnamed - jusedstyles
   ,jmarketprices          :: [MarketPrice]
   ,jtxnmodifiers          :: [TransactionModifier]
   ,jperiodictxns          :: [PeriodicTransaction]
@@ -392,7 +392,7 @@ type StorageFormat = String
 -- which let you walk up or down the account tree.
 data Account = Account {
   aname                     :: AccountName,   -- ^ this account's full name
-  acode                     :: Maybe AccountCode,   -- ^ this account's numeric code, if any (not always set) 
+  adeclarationorder         :: Maybe Int  ,   -- ^ the relative position of this account's account directive, if any. Normally a natural number. 
   aebalance                 :: MixedAmount,   -- ^ this account's balance, excluding subaccounts
   asubs                     :: [Account],     -- ^ sub-accounts
   anumpostings              :: Int,           -- ^ number of postings to this account

hledger-lib/Hledger/Read/JournalReader.hs

@@ -261,8 +261,7 @@ accountdirectivep = do
   string "account"
   lift (skipSome spacenonewline)
   acct <- modifiedaccountnamep  -- account directives can be modified by alias/apply account
-  macode' :: Maybe String <- (optional $ lift $ skipSome spacenonewline >> some digitChar)
-  let macode :: Maybe AccountCode = read <$> macode'
+  _ :: Maybe String <- (optional $ lift $ skipSome spacenonewline >> some digitChar)  -- compatibility: ignore account codes supported in 1.9/1.10
   newline
   skipMany indentedlinep
   pushDeclaredAccount acct

hledger-lib/Hledger/Reports/BalanceReport.hs

@@ -18,6 +18,7 @@ module Hledger.Reports.BalanceReport (
   BalanceReportItem,
   balanceReport,
   flatShowsExclusiveBalance,
+  sortAccountItemsLike, 
 
   -- * Tests
   tests_BalanceReport
@@ -78,7 +79,7 @@ flatShowsExclusiveBalance    = True
 balanceReport :: ReportOpts -> Query -> Journal -> BalanceReport
 balanceReport opts q j = 
   (if invert_ opts then brNegate else id) $ 
-  (items, total)
+  (sorteditems, total)
     where
       -- dbg1 = const id -- exclude from debug output
       dbg1 s = let p = "balanceReport" in Hledger.Utils.dbg1 (p++" "++s)  -- add prefix in debug output
@@ -89,7 +90,6 @@ balanceReport opts q j =
                          dbg1 "accts" $
                          take 1 $ clipAccountsAndAggregate (queryDepth q) $ flattenAccounts accts
           | flat_ opts = dbg1 "accts" $
-                         sortflat $
                          filterzeros $
                          filterempty $
                          drop 1 $ clipAccountsAndAggregate (queryDepth q) $ flattenAccounts accts
@@ -98,27 +98,52 @@ balanceReport opts q j =
                          drop 1 $ flattenAccounts $
                          markboring $
                          prunezeros $
-                         sorttree $
+                         sortAccountTreeByAmount (fromMaybe NormallyPositive $ normalbalance_ opts) $
                          clipAccounts (queryDepth q) accts
           where
-            balance     = if flat_ opts then aebalance else aibalance
+            balance   = if flat_ opts then aebalance else aibalance
             filterzeros = if empty_ opts then id else filter (not . isZeroMixedAmount . balance)
             filterempty = filter (\a -> anumpostings a > 0 || not (isZeroMixedAmount (balance a)))
             prunezeros  = if empty_ opts then id else fromMaybe nullacct . pruneAccounts (isZeroMixedAmount . balance)
             markboring  = if no_elide_ opts then id else markBoringParentAccounts
-            sortflat | sort_amount_ opts = sortBy (maybeflip $ comparing balance)
-                     | otherwise         = sortBy (comparing accountCodeAndNameForSort)
-              where
-                maybeflip = if normalbalance_ opts == Just NormallyNegative then id else flip
-            sorttree | sort_amount_ opts = sortAccountTreeByAmount (fromMaybe NormallyPositive $ normalbalance_ opts)
-                     | otherwise         = sortAccountTreeByAccountCodeAndName
+
       items = dbg1 "items" $ map (balanceReportItem opts q) accts'
+
+      -- now sort items like MultiBalanceReport, except 
+      -- sorting a tree by amount was more easily done above
+      sorteditems 
+        | sort_amount_ opts && tree_ opts = items
+        | sort_amount_ opts               = sortFlatBRByAmount items
+        | otherwise                       = sortBRByAccountDeclaration items
+
+        where    
+          -- Sort the report rows, representing a flat account list, by row total. 
+          sortFlatBRByAmount :: [BalanceReportItem] -> [BalanceReportItem]
+          sortFlatBRByAmount = sortBy (maybeflip $ comparing (normaliseMixedAmountSquashPricesForDisplay . fourth4))
+            where
+              maybeflip = if normalbalance_ opts == Just NormallyNegative then id else flip
+
+          -- Sort the report rows by account declaration order then account name. 
+          sortBRByAccountDeclaration :: [BalanceReportItem] -> [BalanceReportItem]
+          sortBRByAccountDeclaration rows = sortedrows
+            where 
+              anamesandrows = [(first4 r, r) | r <- rows]
+              anames = map fst anamesandrows
+              sortedanames = sortAccountNamesByDeclaration j (tree_ opts) anames
+              sortedrows = sortAccountItemsLike sortedanames anamesandrows 
+
       total | not (flat_ opts) = dbg1 "total" $ sum [amt | (_,_,indent,amt) <- items, indent == 0]
             | otherwise        = dbg1 "total" $
                                  if flatShowsExclusiveBalance
                                  then sum $ map fourth4 items
                                  else sum $ map aebalance $ clipAccountsAndAggregate 1 accts'
 
+-- | A sorting helper: sort a list of things (eg report rows) keyed by account name
+-- to match the provided ordering of those same account names.
+sortAccountItemsLike :: [AccountName] -> [(AccountName, b)] -> [b] 
+sortAccountItemsLike sortedas items =
+  concatMap (\a -> maybe [] (:[]) $ lookup a items) sortedas
+
 -- | In an account tree with zero-balance leaves removed, mark the
 -- elidable parent accounts (those with one subaccount and no balance
 -- of their own).