Merge pull request #39 from longieirl/fix/29-classifier-registry

fix(#29): add ClassifierRegistry with explicit priorities

Author: longieirl

packages/parser-core/src/bankstatements_core/extraction/row_classifiers.py

@@ -9,6 +9,7 @@
 
 import re
 from abc import ABC, abstractmethod
+from typing import Sequence
 
 from bankstatements_core.extraction.column_identifier import ColumnTypeIdentifier
 
@@ -349,37 +350,78 @@ def _do_classify(
         return "metadata"
 
 
+class ClassifierRegistry:
+    """Wires a RowClassifier chain from an explicit priority list.
+
+    Priorities are integers; lower = checked first.
+    No two classifiers may share a priority (raises ValueError at construction).
+    build_chain() returns the standard RowClassifier head — callers are unchanged.
+    """
+
+    def __init__(
+        self,
+        classifiers: Sequence[tuple[int, type[RowClassifier]]],
+    ) -> None:
+        """
+        Args:
+            classifiers: Sequence of (priority, classifier_class) pairs.
+
+        Raises:
+            ValueError: If any two classifiers share the same priority.
+            TypeError: If a class is not a RowClassifier subclass.
+        """
+        seen_priorities: dict[int, str] = {}
+        for priority, cls in classifiers:
+            if not (isinstance(cls, type) and issubclass(cls, RowClassifier)):
+                raise TypeError(f"{cls!r} is not a RowClassifier subclass")
+            if priority in seen_priorities:
+                raise ValueError(
+                    f"priority {priority} already assigned to "
+                    f"{seen_priorities[priority]}, cannot also assign to "
+                    f"{cls.__name__}"
+                )
+            seen_priorities[priority] = cls.__name__
+
+        self._classifiers: list[tuple[int, type[RowClassifier]]] = sorted(
+            classifiers, key=lambda x: x[0]
+        )
+
+    def build_chain(self) -> RowClassifier:
+        """Instantiate classifiers in priority order, wire set_next(), return head."""
+        instances = [cls() for _, cls in self._classifiers]
+        for i in range(len(instances) - 1):
+            instances[i].set_next(instances[i + 1])
+        return instances[0]
+
+    def get_priority_order(self) -> list[tuple[int, str]]:
+        """Return [(priority, class_name), ...] in execution order."""
+        return [(priority, cls.__name__) for priority, cls in self._classifiers]
+
+
 def create_row_classifier_chain() -> RowClassifier:
     """
     Create and configure the chain of row classifiers.
 
-    The order matters - classifiers are applied in sequence:
-    1. HeaderMetadataClassifier - Detect column headers first
-    2. AdministrativeClassifier - Detect administrative entries
-    3. ReferenceCodeClassifier - Detect reference codes
-    4. FXContinuationClassifier - Detect FX continuation lines
-    5. TimestampMetadataClassifier - Detect timestamps
-    6. TransactionClassifier - Detect transactions
-    7. DefaultMetadataClassifier - Catch-all for anything else
+    Priority order (0 = checked first):
+      0  HeaderMetadataClassifier    — column headers and field labels
+      1  AdministrativeClassifier    — BALANCE FORWARD, Lending @
+      2  ReferenceCodeClassifier     — IE123456 patterns
+      3  FXContinuationClassifier    — FX rates, fees, exchange lines
+      4  TimestampMetadataClassifier — 01JAN2023 TIME 14:30
+      5  TransactionClassifier       — debit/credit/date combinations
+      6  DefaultMetadataClassifier   — catch-all fallback
 
     Returns:
         The head of the classifier chain
     """
-    # Create classifiers
-    header_classifier = HeaderMetadataClassifier()
-    admin_classifier = AdministrativeClassifier()
-    reference_classifier = ReferenceCodeClassifier()
-    fx_classifier = FXContinuationClassifier()
-    timestamp_classifier = TimestampMetadataClassifier()
-    transaction_classifier = TransactionClassifier()
-    default_classifier = DefaultMetadataClassifier()
-
-    # Build the chain
-    header_classifier.set_next(admin_classifier)
-    admin_classifier.set_next(reference_classifier)
-    reference_classifier.set_next(fx_classifier)
-    fx_classifier.set_next(timestamp_classifier)
-    timestamp_classifier.set_next(transaction_classifier)
-    transaction_classifier.set_next(default_classifier)
-
-    return header_classifier
+    return ClassifierRegistry(
+        [
+            (0, HeaderMetadataClassifier),
+            (1, AdministrativeClassifier),
+            (2, ReferenceCodeClassifier),
+            (3, FXContinuationClassifier),
+            (4, TimestampMetadataClassifier),
+            (5, TransactionClassifier),
+            (6, DefaultMetadataClassifier),
+        ]
+    ).build_chain()

packages/parser-core/tests/extraction/test_row_classifiers.py

@@ -6,6 +6,7 @@
 
 from bankstatements_core.extraction.row_classifiers import (
     AdministrativeClassifier,
+    ClassifierRegistry,
     DefaultMetadataClassifier,
     FXContinuationClassifier,
     HeaderMetadataClassifier,
@@ -390,3 +391,118 @@ def test_looks_like_date(self):
         assert classifier._looks_like_date("15 December") is True  # Full month name
         assert classifier._looks_like_date("01JAN2023") is True
         assert classifier._looks_like_date("Not a date") is False
+
+
+class TestClassifierRegistry:
+    """Tests for ClassifierRegistry."""
+
+    def test_classifier_priority_order(self):
+        """get_priority_order() reflects the declared priority sequence."""
+        registry = ClassifierRegistry(
+            [
+                (0, HeaderMetadataClassifier),
+                (1, AdministrativeClassifier),
+                (2, ReferenceCodeClassifier),
+                (3, FXContinuationClassifier),
+                (4, TimestampMetadataClassifier),
+                (5, TransactionClassifier),
+                (6, DefaultMetadataClassifier),
+            ]
+        )
+        order = registry.get_priority_order()
+        assert order[0] == (0, "HeaderMetadataClassifier")
+        assert order[5] == (5, "TransactionClassifier")
+        assert order[6] == (6, "DefaultMetadataClassifier")
+
+    def test_duplicate_priority_raises(self):
+        """Duplicate priorities raise ValueError at construction time."""
+        with pytest.raises(ValueError, match="priority 0 already assigned"):
+            ClassifierRegistry(
+                [
+                    (0, HeaderMetadataClassifier),
+                    (0, TransactionClassifier),
+                ]
+            )
+
+    def test_non_classifier_subclass_raises(self):
+        """Passing a non-RowClassifier class raises TypeError."""
+        with pytest.raises(TypeError):
+            ClassifierRegistry([(0, object)])  # type: ignore[list-item]
+
+    def test_build_chain_returns_head(self):
+        """build_chain() returns a RowClassifier instance."""
+        registry = ClassifierRegistry(
+            [
+                (0, HeaderMetadataClassifier),
+                (1, DefaultMetadataClassifier),
+            ]
+        )
+        head = registry.build_chain()
+        assert isinstance(head, RowClassifier)
+        assert isinstance(head, HeaderMetadataClassifier)
+
+    def test_priorities_sorted_regardless_of_input_order(self):
+        """Registry sorts by priority even if input is unordered."""
+        registry = ClassifierRegistry(
+            [
+                (5, TransactionClassifier),
+                (0, HeaderMetadataClassifier),
+                (6, DefaultMetadataClassifier),
+            ]
+        )
+        order = registry.get_priority_order()
+        assert order[0] == (0, "HeaderMetadataClassifier")
+        assert order[1] == (5, "TransactionClassifier")
+        assert order[2] == (6, "DefaultMetadataClassifier")
+
+    @pytest.mark.parametrize(
+        "row,expected,reason",
+        [
+            (
+                {"Date": "date", "Details": "Purchase", "Debit €": "50.00"},
+                "metadata",
+                "HeaderMetadata (0) beats Transaction (5) for header-like date value",
+            ),
+            (
+                {
+                    "Date": "",
+                    "Details": "BALANCE FORWARD",
+                    "Debit €": "",
+                    "Credit €": "",
+                    "Balance €": "",
+                },
+                "administrative",
+                "Administrative (1) beats Transaction (5) for BALANCE FORWARD with no balance",
+            ),
+            (
+                {"Date": "", "Details": "0.828571", "Debit €": "", "Credit €": ""},
+                "continuation",
+                "FXContinuation (3) beats Transaction (5) for exchange-rate-only rows",
+            ),
+        ],
+    )
+    def test_ambiguous_row_priority(self, row, expected, reason):
+        """Ambiguous rows resolve to the highest-priority (lowest number) classifier."""
+        chain = create_row_classifier_chain()
+        assert chain.classify(row, TEST_COLUMNS) == expected, reason
+
+    def test_wrong_order_produces_wrong_result(self):
+        """Documents that priority order is not arbitrary — regression guard."""
+        wrong_order_chain = ClassifierRegistry(
+            [
+                (0, TransactionClassifier),
+                (1, HeaderMetadataClassifier),
+                (2, AdministrativeClassifier),
+                (3, ReferenceCodeClassifier),
+                (4, FXContinuationClassifier),
+                (5, TimestampMetadataClassifier),
+                (6, DefaultMetadataClassifier),
+            ]
+        ).build_chain()
+
+        ambiguous = {"Date": "date", "Details": "Purchase", "Debit €": "50.00"}
+        # With Transaction first, it wins over Header
+        assert wrong_order_chain.classify(ambiguous, TEST_COLUMNS) == "transaction"
+        # Confirms the correct chain must put HeaderMetadata first
+        correct_chain = create_row_classifier_chain()
+        assert correct_chain.classify(ambiguous, TEST_COLUMNS) == "metadata"