From f1f9635c21bf7c8778f5bf5e339268f900d59a1f Mon Sep 17 00:00:00 2001
From: Nils Homer <nilshomer@gmail.com>
Date: Fri, 4 Aug 2023 15:44:34 -0700
Subject: [PATCH] feat: Metric file accepts missing columns if they have
 defaults

---
 fgpyo/util/inspect.py            |  5 +++++
 fgpyo/util/metric.py             | 13 +++++++------
 fgpyo/util/tests/test_inspect.py | 11 +++++++++++
 fgpyo/util/tests/test_metric.py  | 27 +++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/fgpyo/util/inspect.py b/fgpyo/util/inspect.py
index aa458032..6a69a6b5 100644
--- a/fgpyo/util/inspect.py
+++ b/fgpyo/util/inspect.py
@@ -315,3 +315,8 @@ def attribute_is_optional(attribute: attr.Attribute) -> bool:
     return types.get_origin_type(attribute.type) is Union and isinstance(
         None, types.get_arg_types(attribute.type)
     )
+
+
+def attribute_has_default(attribute: attr.Attribute) -> bool:
+    """Returns True if the attribute has a default value, False otherwise"""
+    return attribute.default != attr.NOTHING or attribute_is_optional(attribute)
diff --git a/fgpyo/util/metric.py b/fgpyo/util/metric.py
index c2710daf..9379114c 100644
--- a/fgpyo/util/metric.py
+++ b/fgpyo/util/metric.py
@@ -169,17 +169,18 @@ def read(cls, path: Path, ignore_extra_fields: bool = True) -> Iterator[Any]:
             missing_from_class = file_fields.difference(class_fields)
             missing_from_file = class_fields.difference(file_fields)
 
-            # ignore optional class fields that are missing from the file (via header), they'll get
-            # a default of None
+            field_name_to_attribute = attr.fields_dict(cls)
+
+            # ignore class fields that are missing from the file (via header) if they're optional
+            # or have a default
             if len(missing_from_file) > 0:
-                field_name_to_attribute = attr.fields_dict(cls)
-                missing_from_file_optionals = [
+                fields_with_defaults = [
                     field
                     for field in missing_from_file
-                    if inspect.attribute_is_optional(field_name_to_attribute[field])
+                    if inspect.attribute_has_default(field_name_to_attribute[field])
                 ]
                 # remove optional class fields from the fields
-                missing_from_file = missing_from_file.difference(missing_from_file_optionals)
+                missing_from_file = missing_from_file.difference(fields_with_defaults)
 
             # raise an exception if there are non-optional class fields missing from the file
             if len(missing_from_file) > 0:
diff --git a/fgpyo/util/tests/test_inspect.py b/fgpyo/util/tests/test_inspect.py
index ad9fd3ec..dc5f85a8 100644
--- a/fgpyo/util/tests/test_inspect.py
+++ b/fgpyo/util/tests/test_inspect.py
@@ -1,5 +1,6 @@
 from fgpyo.util.inspect import attribute_is_optional
 from fgpyo.util.inspect import attr_from
+from fgpyo.util.inspect import attribute_has_default
 
 import attr
 from typing import Optional
@@ -30,6 +31,16 @@ def test_attr_from() -> None:
 def test_attribute_is_optional() -> None:
     fields_dict = attr.fields_dict(Name)
     assert not attribute_is_optional(fields_dict["required"])
+    assert not attribute_is_optional(fields_dict["custom_parser"])
     assert attribute_is_optional(fields_dict["optional_no_default"])
     assert attribute_is_optional(fields_dict["optional_with_default_none"])
     assert attribute_is_optional(fields_dict["optional_with_default_some"])
+
+
+def test_attribute_has_default() -> None:
+    fields_dict = attr.fields_dict(Name)
+    assert not attribute_has_default(fields_dict["required"])
+    assert not attribute_has_default(fields_dict["custom_parser"])
+    assert attribute_has_default(fields_dict["optional_no_default"])
+    assert attribute_has_default(fields_dict["optional_with_default_none"])
+    assert attribute_has_default(fields_dict["optional_with_default_some"])
diff --git a/fgpyo/util/tests/test_metric.py b/fgpyo/util/tests/test_metric.py
index e2b37b9d..ab8fd8ad 100644
--- a/fgpyo/util/tests/test_metric.py
+++ b/fgpyo/util/tests/test_metric.py
@@ -131,6 +131,12 @@ class PersonMaybeAge(Metric["PersonMaybeAge"]):
     age: Optional[int]
 
 
+@attr.s(auto_attribs=True, frozen=True)
+class PersonDefault(Metric["PersonDefault"]):
+    name: str
+    age: int = 0
+
+
 @pytest.mark.parametrize("metric", DUMMY_METRICS)
 def test_metric_roundtrip(tmpdir: TmpDir, metric: DummyMetric) -> None:
     path: Path = Path(tmpdir) / "metrics.txt"
@@ -183,6 +189,27 @@ def test_metrics_read_missing_optional_columns(tmpdir: TmpDir) -> None:
         list(PersonMaybeAge.read(path=path))
 
 
+def test_metric_read_missing_column_with_default(tmpdir: TmpDir) -> None:
+    person = PersonDefault(name="Max")
+    path = Path(tmpdir) / "metrics.txt"
+
+    # The "age" column hs a default, and not in the file, but that's ok
+    with path.open("w") as writer:
+        writer.write("name\nMax\n")
+    assert list(PersonDefault.read(path=path)) == [person]
+
+    # All fields specified
+    with path.open("w") as writer:
+        writer.write("name\tage\nMax\t42\n")
+    assert list(PersonDefault.read(path=path)) == [PersonDefault(name="Max", age=42)]
+
+    # Just age specified, but not the required name column!
+    with path.open("w") as writer:
+        writer.write("age\n42\n")
+    with pytest.raises(ValueError):
+        list(PersonDefault.read(path=path))
+
+
 def test_metric_header() -> None:
     assert DummyMetric.header() == [
         "int_value",