Parse json numbers directly as Decimals in Schwab Equity Award parser. (

#307)

* Parse JSON numbers directly as Decimals.

We use json.load() custom parser options. This is simpler and safer than
the previous default loading as floats, then converted to Decimals as this
exposes us to the imprecision of floats.

* Fix typo in comment.

Co-authored-by: Cosimo Lupo <[email protected]>

* Add comment back

Co-authored-by: Cosimo Lupo <[email protected]>
Co-authored-by: Ruslan Sayfutdinov <[email protected]>

cgt_calc/parsers/schwab_equity_award_json.py

@@ -93,38 +93,34 @@ def action_from_str(label: str) -> ActionType:
     raise ParsingError("schwab transactions", f"Unknown action: {label}")
 
 
-def _get_decimal_or_default(
-    row: JsonRowType, key: str, default: Decimal | None = None
-) -> Decimal | None:
-    if key in row and row[key]:
-        if isinstance(row[key], float):
-            return round_decimal(Decimal.from_float(row[key]), ROUND_DIGITS)
+def _decimal_from_str(price_str: str) -> Decimal:
+    """Convert a number as string to a Decimal.
 
-        return Decimal(row[key])
-
-    return default
-
-
-def _get_decimal(row: JsonRowType, key: str) -> Decimal:
-    return _get_decimal_or_default(row, key, Decimal(0))  # type: ignore
-
-
-def _price_from_str(price_str: str) -> Decimal:
-    # example: "$1,250.00",
-    # remove $ sign, and coma thousand separators:
+    Remove $ sign, and comma thousand separators so as to handle dollar amounts
+    such as "$1,250.00".
+    """
     return Decimal(price_str.replace("$", "").replace(",", ""))
 
 
-def _price_from_str_or_float(
+def _decimal_from_number_or_str(
     row: JsonRowType,
     field_basename: str,
     field_float_suffix: str = "SortValue",
 ) -> Decimal:
-    # We prefer strings to floats which get rounded when parsed:
-    if field_basename in row and row[field_basename]:
-        return _price_from_str(row[field_basename])
+    """Get a number from a row, preferably from the number field.
+
+    Fall back to the string representation field, or default to Decimal(0)
+    if the fields are not there or both have a value of None.
+    """
+    # We prefer native number to strings as more efficient/safer parsing
+    float_name = f"{field_basename}{field_float_suffix}"
+    if float_name in row and row[float_name] is not None:
+        return Decimal(row[float_name])
 
-    return _get_decimal(row, f"{field_basename}{field_float_suffix}")
+    if field_basename in row and row[field_basename] is not None:
+        return _decimal_from_str(row[field_basename])
+
+    return Decimal(0)
 
 
 def _is_integer(number: Decimal) -> bool:
@@ -144,9 +140,9 @@ def __init__(
         self.raw_action = row["action"]
         action = action_from_str(self.raw_action)
         symbol = row.get("symbol")
-        quantity = _price_from_str_or_float(row, "quantity")
-        amount = _price_from_str_or_float(row, "amount")
-        fees = _price_from_str_or_float(row, "totalCommissionsAndFees")
+        quantity = _decimal_from_number_or_str(row, "quantity")
+        amount = _decimal_from_number_or_str(row, "amount")
+        fees = _decimal_from_number_or_str(row, "totalCommissionsAndFees")
         if row["action"] == "Deposit":
             if len(row["transactionDetails"]) != 1:
                 raise ParsingError(
@@ -157,7 +153,10 @@ def __init__(
             date = datetime.datetime.strptime(
                 row["transactionDetails"][0]["vestDate"], "%m/%d/%Y"
             ).date()
-            price = _price_from_str(row["transactionDetails"][0]["vestFairMarketValue"])
+            # Schwab only provide this one as string:
+            price = _decimal_from_str(
+                row["transactionDetails"][0]["vestFairMarketValue"]
+            )
             if amount == Decimal(0):
                 amount = price * quantity
             description = (
@@ -184,7 +183,8 @@ def __init__(
                 found_share_decimals = False
                 for subtransac in row["transactionDetails"]:
                     if "shares" in subtransac:
-                        shares = _price_from_str(subtransac["shares"])
+                        # Schwab only provides this one as a string:
+                        shares = _decimal_from_str(subtransac["shares"])
                         subtransac_shares_sum += shares
                         if not _is_integer(shares):
                             found_share_decimals = True
@@ -201,7 +201,7 @@ def __init__(
                     # We can only work-out the correct quantity if all
                     # sub-transactions have the same price:
                     price_str = row["transactionDetails"][0]["salePrice"]
-                    price = _price_from_str(price_str)
+                    price = _decimal_from_str(price_str)
 
                     for subtransac in row["transactionDetails"][1:]:
                         if subtransac["salePrice"] != price_str:
@@ -266,7 +266,7 @@ def read_schwab_equity_award_json_transactions(
     try:
         with Path(transactions_file).open(encoding="utf-8") as json_file:
             try:
-                data = json.load(json_file)
+                data = json.load(json_file, parse_float=Decimal, parse_int=Decimal)
             except json.decoder.JSONDecodeError as exception:
                 raise ParsingError(
                     transactions_file,

tests/test_schwab_equity_award_json.py

@@ -8,73 +8,54 @@
 from cgt_calc.parsers import schwab_equity_award_json
 
 
-def test_get_decimal_or_default_present_int() -> None:
-    """Test get_decimal_or_default() on an int."""
-    assert schwab_equity_award_json._get_decimal_or_default(  # pylint: disable=W0212
-        {"key": 1}, "key"
-    ) == Decimal(1)
-
-
-def test_get_decimal_or_default_present_float() -> None:
-    """Test get_decimal_or_default() on a float."""
-    assert schwab_equity_award_json._get_decimal_or_default(  # pylint: disable=W0212
-        {"key": 1.0}, "key"
-    ) == Decimal(1.0)
-
-
-def test_get_decimal_or_default_absent() -> None:
-    """Test get_decimal_or_default() on absent key."""
-    assert schwab_equity_award_json._get_decimal_or_default(  # pylint: disable=W0212
-        {"key": 1}, "otherkey", Decimal(0)
-    ) == Decimal(0)
-
-
-def test_price_from_str() -> None:
-    """Test _price_from_str()."""
-    assert schwab_equity_award_json._price_from_str(  # pylint: disable=W0212
+def test_decimal_from_str() -> None:
+    """Test _decimal_from_str()."""
+    assert schwab_equity_award_json._decimal_from_str(  # pylint: disable=W0212
         "$123,456.23"
     ) == Decimal("123456.23")
 
 
-def test_price_from_str_or_float_str() -> None:
-    """Test _price_from_str_or_float() on string."""
-    assert schwab_equity_award_json._price_from_str_or_float(  # pylint: disable=W0212
-        {"key": "123.45", "keySortValue": 67.89}, "key"
-    ) == Decimal("123.45")
-
-
-def test_price_from_str_or_float_str_null() -> None:
-    """Test _price_from_str_or_float() on None string."""
-    assert schwab_equity_award_json._price_from_str_or_float(  # pylint: disable=W0212
-        {"key": None, "keySortValue": 67.89}, "key"
-    ) == Decimal("67.89")
-
+def test_decimal_from_number_or_str_both() -> None:
+    """Test _decimal_from_number_or_str() on float."""
+    assert (
+        schwab_equity_award_json._decimal_from_number_or_str(  # pylint: disable=W0212
+            {"key": "123.45", "keySortValue": Decimal("67.89")}, "key"
+        )
+        == Decimal("67.89")
+    )
 
-def test_price_from_str_or_float_float_default_suffix() -> None:
-    """Test _price_from_str_or_float_default_suffix() on float.
 
-    With the default suffix.
-    """
-    assert schwab_equity_award_json._price_from_str_or_float(  # pylint: disable=W0212
-        {"keySortValue": 67.89}, "key"
-    ) == Decimal("67.89")
+def test_decimal_from_number_or_str_float_null() -> None:
+    """Test _decimal_from_number_or_str() on None float."""
+    assert (
+        schwab_equity_award_json._decimal_from_number_or_str(  # pylint: disable=W0212
+            {"key": "67.89", "keySortValue": None}, "key"
+        )
+        == Decimal("67.89")
+    )
 
 
-def test_price_from_str_or_float_float_custom_suffix() -> None:
-    """Test _price_from_str_or_float_default_suffix() on float.
+def test_decimal_from_number_or_str_float_custom_suffix() -> None:
+    """Test _decimal_from_number_or_str_default_suffix() on float.
 
     With a custom suffix.
     """
-    assert schwab_equity_award_json._price_from_str_or_float(  # pylint: disable=W0212
-        {"keyMySuffix": 67.89}, "key", "MySuffix"
-    ) == Decimal("67.89")
+    assert (
+        schwab_equity_award_json._decimal_from_number_or_str(  # pylint: disable=W0212
+            {"keyMySuffix": Decimal("67.89")}, "key", "MySuffix"
+        )
+        == Decimal("67.89")
+    )
 
 
-def test_price_from_str_or_float_default() -> None:
-    """Test _price_from_str_or_float() with absent keys."""
-    assert schwab_equity_award_json._price_from_str_or_float(  # pylint: disable=W0212
-        {"key": "123.45", "keySortValue": 67.89}, "otherkey"
-    ) == Decimal("0")
+def test_decimal_from_number_or_str_default() -> None:
+    """Test _decimal_from_number_or_str() with absent keys."""
+    assert (
+        schwab_equity_award_json._decimal_from_number_or_str(  # pylint: disable=W0212
+            {"key": "123.45", "keySortValue": 67.89}, "otherkey"
+        )
+        == Decimal("0")
+    )
 
 
 def test_schwab_transaction() -> None: