reconcile: Fix a mutation bug causing not all matches to be passed through.

conservancy_beancount/reconcile/statement_reconciler.py

@@ -104,12 +104,15 @@ import logging
 import os
 import re
 import sys
-from typing import Callable, Dict, List, Tuple, TextIO
+from typing import Callable, Dict, List, Optional, Sequence, Tuple, TextIO
 
 from beancount import loader
 from beancount.query.query import run_query
 from colorama import Fore, Style  # type: ignore
 
+from .. import cliutil
+from .. import config as configmod
+
 if not sys.warnoptions:
     import warnings
     # Disable annoying warning from thefuzz prompting for a C extension. The
@@ -117,12 +120,12 @@ if not sys.warnoptions:
     warnings.filterwarnings('ignore', category=UserWarning, module='thefuzz.fuzz')
 from thefuzz import fuzz  # type: ignore
 
-logger = logging.getLogger()
+PROGNAME = 'reconcile-statement'
-logger.setLevel(logging.INFO)
+logger = logging.getLogger(__name__)
-
-# Console logging.
-logger.addHandler(logging.StreamHandler())
 
+# Get some interesting feedback on call to RT with this:
+# logger.setLevel(logging.DEBUG)
+# logger.addHandler(logging.StreamHandler())
 
 JUNK_WORDS = [
     'software',
@@ -457,8 +460,16 @@ def parse_repo_relative_path(path: str) -> str:
     return path
 
 
-def parse_args(argv: List[str]) -> argparse.Namespace:
+def parse_decimal_with_separator(number_text: str) -> decimal.Decimal:
-    parser = argparse.ArgumentParser(description='Reconciliation helper')
+    """decimal.Decimal can't parse numbers with thousands separator."""
+    number_text = number_text.replace(',', '')
+    return decimal.Decimal(number_text)
+
+
+def parse_arguments(argv: List[str]) -> argparse.Namespace:
+    parser = argparse.ArgumentParser(prog=PROGNAME, description='Reconciliation helper')
+    cliutil.add_version_argument(parser)
+    cliutil.add_loglevel_argument(parser)
     parser.add_argument('--beancount-file', required=True, type=parse_path)
     parser.add_argument('--csv-statement', required=True, type=parse_repo_relative_path)
     parser.add_argument('--bank-statement', required=True, type=parse_repo_relative_path)
@@ -466,9 +477,10 @@ def parse_args(argv: List[str]) -> argparse.Namespace:
     parser.add_argument('--grep-output-filename')
     # parser.add_argument('--report-group-regex')
     parser.add_argument('--show-reconciled-matches', action='store_true')
-    parser.add_argument('--statement-balance', type=decimal.Decimal, required=True, help="A.K.A \"cleared balance\" taken from the end of the period on the PDF statement. Required because CSV statements don't include final or running totals")
+    parser.add_argument('--statement-balance', type=parse_decimal_with_separator, required=True, help="A.K.A \"cleared balance\" taken from the end of the period on the PDF statement. Required because CSV statements don't include final or running totals")
     parser.add_argument('--non-interactive', action='store_true', help="Don't prompt to write to the books")
-    return parser.parse_args(args=argv[1:])
+    args = parser.parse_args(args=argv)
+    return args
 
 
 def totals(matches: List[Tuple[List, List, List]]) -> Tuple[decimal.Decimal, decimal.Decimal, decimal.Decimal]:
@@ -512,9 +524,6 @@ def subset_match(statement_trans: List[dict], books_trans: List[dict]) -> Tuple[
             matches.append(([statement_trans[best_match_index]], group_items, best_match_note))
             if best_match_index is not None:
                 del statement_trans[best_match_index]
-            for item in group_items:
-                # TODO: Why?
-                books_trans.remove(item)
         else:
             remaining_books_trans.append(r2)
     for r1 in statement_trans:
@@ -531,7 +540,17 @@ def process_unmatched(statement_trans: List[dict], books_trans: List[dict]) -> L
     return matches
 
 
-def main(args: argparse.Namespace) -> None:
+def main(arglist: Optional[Sequence[str]] = None,
+         stdout: TextIO = sys.stdout,
+         stderr: TextIO = sys.stderr,
+         config: Optional[configmod.Config] = None,
+         ) -> int:
+    args = parse_arguments(arglist)
+    cliutil.set_loglevel(logger, args.loglevel)
+    if config is None:
+        config = configmod.Config()
+        config.load_file()
+
     # TODO: Should put in a sanity check to make sure the statement you're feeding
     # in matches the account you've provided.
 
@@ -607,10 +626,7 @@ def main(args: argparse.Namespace) -> None:
             write_metadata_to_books(metadata_to_apply)
 
 
-if __name__ == '__main__':
+entry_point = cliutil.make_entry_point(__name__, PROGNAME)
-    args = parse_args(sys.argv)
-    main(args)
 
-def entry_point():
+if __name__ == '__main__':
-    args = parse_args(sys.argv)
+    exit(entry_point())
-    main(args)

tests/test_reconcile.py

@@ -308,3 +308,21 @@ def test_subset_sum_match():
         [],  # No remaining statement trans.
         [],  # No remaining books trans.
     )
+
+def test_subset_passes_through_all_non_matches():
+    """This was used to locate a bug where some of the non-matches had
+    gone missing due to mutation of books_trans."""
+    statement_trans = [
+        S1,  # No match
+        S4,  # Match
+    ]
+    books_trans = [
+        B2,  # No match
+        B4A, B4B, B4C, # Match
+        B3_next_day, B3_next_week,  # No match
+    ]
+    assert subset_match(statement_trans, books_trans) == (
+        [([S4], [B4A, B4B, B4C], [])],  # Matched
+        [S1],  # No match: preserved intact
+        [B2, B3_next_day, B3_next_week]  # No match: preserved intact
+    )