Respond to review comments:

* Improve codecov
* Update mypy type ignore statements
* Fix readthedocs
* Fix merge conflicts
* Update poetry then fix black issues
* Added some tests test
* made a few variables and functions private

Author: TedBrookings

codecov.yml

@@ -0,0 +1,2 @@
+ignore:
+  - "**/test_*.py"       # don't compute coverage of tests

fgpyo/sam/__init__.py

@@ -398,7 +398,7 @@ class CigarElement:
     length: int
     operator: CigarOp
 
-    def __post_init__(self) -> None:
+    def __attrs_post_init__(self) -> None:
         """Validates the length attribute is greater than zero."""
         if self.length <= 0:
             raise ValueError(f"Cigar element must have a length > 0, found {self.length}")

fgpyo/sam/tests/test_sam.py

@@ -195,6 +195,12 @@ def test_cigar_element_length_on(
     assert element.length_on_target == length_on_target
 
 
+@pytest.mark.parametrize("character", ["M", "I", "D", "S"])
+def test_invalid_cigar_element(character: str) -> None:
+    with pytest.raises(ValueError):
+        CigarElement(-1, operator=CigarOp.from_character(character))
+
+
 @pytest.mark.parametrize(
     "cigartuples,cigarstring",
     [

fgpyo/util/inspect.py

@@ -1,22 +1,17 @@
-import functools
 import sys
 import typing
-from enum import Enum
-from functools import partial
-from pathlib import PurePath
 from typing import Any
-from typing import Callable
 from typing import Dict
 from typing import Iterable
 from typing import List
 from typing import Literal
-from typing import Optional
+from typing import Mapping
+from typing import Protocol
 from typing import Tuple
 from typing import Type
 from typing import Union
-from typing import Literal
-from typing import Protocol
-if sys.version_info >= (3, 12):
+
+if sys.version_info >= (3, 10):
     from typing import TypeAlias
 else:
     from typing_extensions import TypeAlias
@@ -45,20 +40,28 @@
     from attr import fields_dict as get_attr_fields_dict
 
     Attribute = attr.Attribute
-
+    # dataclasses and attr have internal tokens for missing values, join into a set so that we can
+    # check if a value is missing without knowing the type of backing class
     MISSING = {DATACLASSES_MISSING, ATTR_NOTHING}
 except ImportError:
     _use_attr = False
     attr = None
     ATTR_NOTHING = None
-    Attribute = TypeVar("Attribute", bound=object)  # type: ignore
+    Attribute = TypeVar("Attribute", bound=object)  # type: ignore[misc, assignment]
+
+    # define empty placeholders for getting attr fields as a tuple or dict. They will never be
+    # called because the import failed; but they're here to ensure that the function is defined in
+    # sections of code that don't know if the import was successful or not.
 
-    def get_attr_fields(cls: type) -> Tuple[dataclasses.Field, ...]:  # type: ignore
+    def get_attr_fields(cls: type) -> Tuple[dataclasses.Field, ...]:  # type: ignore[misc]
+        """Get tuple of fields for attr class. attrs isn't imported so return empty tuple."""
         return ()
 
-    def get_attr_fields_dict(cls: type) -> Dict[str, dataclasses.Field]:  # type: ignore
+    def get_attr_fields_dict(cls: type) -> Dict[str, dataclasses.Field]:  # type: ignore[misc]
+        """Get dict of name->field for attr class. attrs isn't imported so return empty dict."""
         return {}
 
+    # for consistency with successful import of attr, create a set for missing values
     MISSING = {DATACLASSES_MISSING}
 
 if TYPE_CHECKING:
@@ -73,22 +76,33 @@ class DataclassesProtocol(Protocol):
     from attr import AttrsInstance
 else:
 
-    class AttrsInstance(Protocol):  # type: ignore
+    class AttrsInstance(Protocol):  # type: ignore[no-redef]
         __attrs_attrs__: Dict[str, Any]
 
 
-def is_attr_class(cls: type) -> bool:  # type: ignore
+def is_attr_class(cls: type) -> bool:  # type: ignore[arg-type]
+    """Return True if the class is an attr class, and False otherwise"""
     return hasattr(cls, "__attrs_attrs__")
 
 
-MISSING_OR_NONE = {*MISSING, None}
-DataclassesOrAttrClass = Union[DataclassesProtocol, AttrsInstance]
-FieldType: TypeAlias = Union[dataclasses.Field, attr.Attribute]
+_MISSING_OR_NONE = {*MISSING, None}
+"""Set of values that are considered missing or None for dataclasses or attr classes"""
+_DataclassesOrAttrClass: TypeAlias = Union[DataclassesProtocol, AttrsInstance]
+"""
+TypeAlias for dataclasses or attr classes. Mostly nonsense because they are not true types, they
+are traits, but there is no python trait-tester.
+"""
+FieldType: TypeAlias = Union[dataclasses.Field, Attribute]
+"""
+TypeAlias for dataclass Fields or attrs Attributes. It will correspond to the correct type for the
+corresponding _DataclassesOrAttrClass
+"""
 
 
-def get_dataclasses_fields_dict(
+def _get_dataclasses_fields_dict(
     class_or_instance: Union[DataclassesProtocol, Type[DataclassesProtocol]],
 ) -> Dict[str, dataclasses.Field]:
+    """Get a dict from field name to Field for a dataclass class or instance."""
     return {field.name: field for field in get_dataclasses_fields(class_or_instance)}
 
 
@@ -372,46 +386,49 @@ def get_parser() -> partial:
     return parser
 
 
-def get_fields_dict(cls: Type[DataclassesOrAttrClass]) -> Dict[str, FieldType]:
-    """
-    Get the fields dict from either a dataclasses or attr dataclass.
+def get_fields_dict(
+    cls: Union[_DataclassesOrAttrClass, Type[_DataclassesOrAttrClass]]
+) -> Mapping[str, FieldType]:
+    """Get the fields dict from either a dataclasses or attr dataclass (or instance)"""
+    if is_dataclasses_class(cls):
+        return _get_dataclasses_fields_dict(cls)  # type: ignore[arg-type]
+    elif is_attr_class(cls):  # type: ignore[arg-type]
+        return get_attr_fields_dict(cls)  # type: ignore[arg-type]
+    else:
+        raise TypeError("cls must a dataclasses or attr class")
 
-    Combine results in case someone chooses to mix them through inheritance.
-    """
-    if not (is_dataclasses_class(cls) or is_attr_class(cls)):
-        raise ValueError("cls must a dataclasses or attr class")
-    return {
-        **(get_dataclasses_fields_dict(cls) if is_dataclasses_class(cls) else {}),
-        **(get_attr_fields_dict(cls) if is_attr_class(cls) else {}),  # type: ignore
-    }
-
-
-def get_fields(cls: Type[DataclassesOrAttrClass]) -> Tuple[FieldType, ...]:
-    if not (is_dataclasses_class(cls) or is_attr_class(cls)):
-        raise ValueError("cls must a dataclasses or attr class")
-    return (get_dataclasses_fields(cls) if is_dataclasses_class(cls) else ()) + (
-        get_attr_fields(cls) if is_attr_class(cls) else ()  # type: ignore
-    )
+
+def get_fields(
+    cls: Union[_DataclassesOrAttrClass, Type[_DataclassesOrAttrClass]]
+) -> Tuple[FieldType, ...]:
+    """Get the fields tuple from either a dataclasses or attr dataclass (or instance)"""
+    if is_dataclasses_class(cls):
+        return get_dataclasses_fields(cls)  # type: ignore[arg-type]
+    elif is_attr_class(cls):  # type: ignore[arg-type]
+        return get_attr_fields(cls)  # type: ignore[arg-type]
+    else:
+        raise TypeError("cls must a dataclasses or attr class")
 
 
-AttrFromType = TypeVar("AttrFromType")
+_AttrFromType = TypeVar("_AttrFromType")
+"""TypeVar to allow attr_from to be used with either an attr class or a dataclasses class"""
 
 
 def attr_from(
-    cls: Type[AttrFromType],
+    cls: Type[_AttrFromType],
     kwargs: Dict[str, str],
     parsers: Optional[Dict[type, Callable[[str], Any]]] = None,
-) -> AttrFromType:
-    """Builds an attr class from key-word arguments
+) -> _AttrFromType:
+    """Builds an attr or dataclasses class from key-word arguments
 
     Args:
-        cls: the attr class to be built
+        cls: the attr or dataclasses class to be built
         kwargs: a dictionary of keyword arguments
         parsers: a dictionary of parser functions to apply to specific types
 
     """
     return_values: Dict[str, Any] = {}
-    for attribute in get_fields(cls):  # type: ignore
+    for attribute in get_fields(cls):  # type: ignore[arg-type]
         return_value: Any
         if attribute.name in kwargs:
             str_value: str = kwargs[attribute.name]
@@ -447,7 +464,7 @@ def attr_from(
                 set_value
             ), f"Do not know how to convert string to {attribute.type} for value: {str_value}"
         else:  # no value, check for a default
-            assert attribute.default is not None or attribute_is_optional(
+            assert attribute.default is not None or _attribute_is_optional(
                 attribute
             ), f"No value given and no default for attribute `{attribute.name}`"
             return_value = attribute.default
@@ -460,13 +477,13 @@ def attr_from(
     return cls(**return_values)
 
 
-def attribute_is_optional(attribute: FieldType) -> bool:
+def _attribute_is_optional(attribute: FieldType) -> bool:
     """Returns True if the attribute is optional, False otherwise"""
     return typing.get_origin(attribute.type) is Union and isinstance(
         None, typing.get_args(attribute.type)
     )
 
 
-def attribute_has_default(attribute: FieldType) -> bool:
+def _attribute_has_default(attribute: FieldType) -> bool:
     """Returns True if the attribute has a default value, False otherwise"""
-    return attribute.default not in MISSING_OR_NONE or attribute_is_optional(attribute)
+    return attribute.default not in _MISSING_OR_NONE or _attribute_is_optional(attribute)

fgpyo/util/metric.py

@@ -10,9 +10,11 @@
 
 The :class:`~fgpyo.util.metric.Metric` class makes it easy to read, write, and store one or metrics
 of the same type, all the while preserving types for each value in a metric.  It is an abstract
-base class decorated by `dataclassees <https://docs.python.org/3/library/dataclasses.html>`_, or
-`attr <https://www.attrs.org/en/stable/examples.html>`_, with attributes storing one or more typed
-values.
+base class decorated by `@dataclass <https://docs.python.org/3/library/dataclasses.html>`_, or
+`@attr.s <https://www.attrs.org/en/stable/examples.html>`_, with attributes storing one or more
+typed values. If using multiple layers of inheritance, keep in mind that it's not possible to mix
+these dataclass utils, e.g. a dataclasses class derived from an attr class will not appropriately
+initialize the values of the attr superclass.
 
 Examples
 ~~~~~~~~
@@ -29,6 +31,7 @@
    ...     age: int
 
 or using attr:
+
 .. code-block:: python
 
    >>> from fgpyo.util.metric import Metric
@@ -112,6 +115,7 @@
    ["first last", "42"]
 
 """
+
 from abc import ABC
 from enum import Enum
 from pathlib import Path
@@ -142,7 +146,7 @@ class Metric(ABC, Generic[MetricType]):
 
     def values(self) -> Iterator[Any]:
         """An iterator over attribute values in the same order as the header."""
-        for field in inspect.get_fields(self.__class__):  # type: ignore
+        for field in inspect.get_fields(self.__class__):  # type: ignore[arg-type]
             yield getattr(self, field.name)
 
     def formatted_values(self) -> List[str]:
@@ -178,15 +182,15 @@ 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)
 
-            field_name_to_attribute = inspect.get_fields_dict(cls)  # type: ignore
+            field_name_to_attribute = inspect.get_fields_dict(cls)  # type: ignore[arg-type]
 
             # 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:
                 fields_with_defaults = [
                     field
                     for field in missing_from_file
-                    if inspect.attribute_has_default(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(fields_with_defaults)
@@ -258,7 +262,7 @@ def write(cls, path: Path, *values: MetricType) -> None:
     @classmethod
     def header(cls) -> List[str]:
         """The list of header values for the metric."""
-        return [a.name for a in inspect.get_fields(cls)]  # type: ignore
+        return [a.name for a in inspect.get_fields(cls)]  # type: ignore[arg-type]
 
     @classmethod
     def format_value(cls, value: Any) -> str: