chore: Modernize mypy config #259
Conversation
feat: build-docs task
| elif type_ is dict: | ||
| raise ValueError("Unable to parse dict (try typing.Mapping[type])") from ex | ||
| elif typing.get_origin(type_) is list: | ||
| elif typing.get_origin(type_) is list: # type: ignore[comparison-overlap] |
There was a problem hiding this comment.
This type ignore is necessary because typing.get_origin may return a type or a ParamSpec. With strict_equality = true, mypy raises comparison-overlap when attempting to use is with two operands of different types.
| return self.record(None, None) | ||
| else: | ||
| return self.record(rec.reference_name, rec.reference_start + 1) | ||
| return self.record(rec.reference_name, rec.reference_start + 1) |
There was a problem hiding this comment.
AlignedSegment.reference_start is of type int, so the if block was unreachable.
There was a problem hiding this comment.
I think we'll need to guard against reference_start being negative, since a start is non-sensical when a read is unmapped and unplaced (e.g. next to its mate). I think that's what the None check was trying to do. What do you think?
| from pysam import FastxFile | ||
| from pysam import FastxRecord | ||
|
|
||
| from fgpyo.util.types import all_not_none |
There was a problem hiding this comment.
I tried to address the type errors with minimal changes to the code, but here adding a helper to narrow the type of a sequence of Optional[T] to T seemed like the least bad of the available options.
msto
force-pushed
the
ms_224_update-mypy
branch
2 times, most recently
from
9c8749d to
c8e943a
Compare
msto
force-pushed
the
ms_224_update-mypy
branch
from
c8e943a to
33a6b60
Compare
msto
force-pushed
the
ms_224_update-mypy
branch
from
33a6b60 to
e4ee2b9
Compare
|
Mypy checks are failing on 3.9. I wonder if this is related to our decision to not enforce a python version in the mypy config. TBH I'd prefer to drop support for 3.9 (#247) since it EOLs next month anyways, rather than spend time fixing. |
|
|
||
| def all_not_none(values: Iterable[Optional[T]]) -> bool: | ||
| """ | ||
| Type guard that checks all Optional collection elements are non-null. |
There was a problem hiding this comment.
| Type guard that checks all Optional collection elements are non-null. | |
| Type guard that checks all Optional collection elements are not None. |
| non_null_names: List[Optional[str]] = [ | ||
| record.name for record in records if record is not None | ||
| ] | ||
| assert all_not_none(non_null_names) # type narrowing | ||
| # We know there is at least one non-null record because the previous conditional covers | ||
| # the case where all records are null, so it is safe to index into the first element of | ||
| # non_null_names. | ||
| sequence_name: str = non_null_names[0] |
There was a problem hiding this comment.
There isn't a null in Python, so I'd replace null with None in this section.
| names_with_ordinals: List[Optional[str]] = [record.name for record in records] | ||
|
|
||
| assert all_not_none(names_with_ordinals) # type narrowing | ||
| record_names: List[str] = [self._name_minus_ordinal(name) for name in names_with_ordinals] | ||
|
|
||
| if len(set(record_names)) != 1: | ||
| raise ValueError(f"FASTX record names do not all match, found: {record_names}") |
There was a problem hiding this comment.
I think this could be accomplished in a single loop, and given this is called on every record, I wonder if we should be concerned with performance here. I.e. run through a loop and check the return of Set.add. Perhaps we should create an issue so that we let folks know we're aware that this could be slowwwww.
| return self.record(None, None) | ||
| else: | ||
| return self.record(rec.reference_name, rec.reference_start + 1) | ||
| return self.record(rec.reference_name, rec.reference_start + 1) |
There was a problem hiding this comment.
I think we'll need to guard against reference_start being negative, since a start is non-sensical when a read is unmapped and unplaced (e.g. next to its mate). I think that's what the None check was trying to do. What do you think?
msto
force-pushed
the
ms_224_modernize-pyproject
branch
from
d2d8475 to
27922cf
Compare
Summary
This PR adds the mypy configuration used by our template and addresses resulting type errors.
Most of the changes are a consequence of setting
strict_optional = true, a setting recommended by the mypy developers.