Repository: matthewwardrop/formulaic

Issue:
Explicitly passed terms should be sorted like parsed terms.
When Formula receives a string as input, it sorts the terms when preparing the items. This is inconsistent because none of `Structured`, `List`, or `Set` sort their terms. String formulas should likewise not be sorting their terms to maintain consistency.

Discussion:

Patch:
diff --git a/formulaic/formula.py b/formulaic/formula.py
index 3a9044b..d4e3a41 100644
--- a/formulaic/formula.py
+++ b/formulaic/formula.py
@@ -137,7 +137,9 @@ class Formula(Structured[List[Term]]):
             )
 
         if isinstance(item, Structured):
-            formula_or_terms = Formula(_parser=self._nested_parser, **item._structure)
+            formula_or_terms = Formula(
+                _parser=self._nested_parser, **item._structure
+            )._simplify()
         elif isinstance(item, (list, set)):
             formula_or_terms = [
                 term
@@ -148,19 +150,17 @@ class Formula(Structured[List[Term]]):
                     else [value]
                 )
             ]
+            self.__validate_terms(formula_or_terms)
+            formula_or_terms = sorted(formula_or_terms)
         else:
             raise FormulaInvalidError(
                 f"Unrecognized formula specification: {repr(item)}."
             )
 
-        self.__validate_prepared_item(formula_or_terms)
-
-        if isinstance(formula_or_terms, Structured):
-            formula_or_terms = formula_or_terms._simplify()
         return formula_or_terms
 
     @classmethod
-    def __validate_prepared_item(cls, formula_or_terms: Any):
+    def __validate_terms(cls, formula_or_terms: Any):
         """
         Verify that all terms are of the appropriate type. The acceptable types
         are:
@@ -168,9 +168,6 @@ class Formula(Structured[List[Term]]):
             - Tuple[List[Terms], ...]
             - Formula
         """
-        if isinstance(formula_or_terms, Formula):
-            formula_or_terms._map(cls.__validate_prepared_item)
-            return
         if not isinstance(formula_or_terms, list):
             # Should be impossible to reach this; here as a sentinel
             raise FormulaInvalidError(


Test patch:
diff --git a/tests/test_formula.py b/tests/test_formula.py
index a54c7be..c1062ea 100644
--- a/tests/test_formula.py
+++ b/tests/test_formula.py
@@ -31,10 +31,10 @@ class TestFormula:
     def test_constructor(self):
         assert [str(t) for t in Formula(["a", "b", "c"])] == ["a", "b", "c"]
         assert [str(t) for t in Formula(["a", "c", "b", "1"])] == [
+            "1",
             "a",
-            "c",
             "b",
-            "1",
+            "c",
         ]
 
         f = Formula((["a", "b"], ["c", "d"]))
@@ -141,7 +141,7 @@ class TestFormula:
             Formula([{"a": 1}])
         with pytest.raises(FormulaInvalidError):
             # Should not be possible to reach this, but check anyway.
-            Formula._Formula__validate_prepared_item(("a",))
+            Formula._Formula__validate_terms(("a",))
 
     def test_invalid_materializer(self, formula_expr, data):
         with pytest.raises(FormulaMaterializerInvalidError):