chore: Some typing-related improvements #1860
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1860 +/- ##
==========================================
+ Coverage 73.36% 73.39% +0.03%
==========================================
Files 80 80
Lines 8752 8762 +10
Branches 1784 1784
==========================================
+ Hits 6421 6431 +10
Misses 2026 2026
Partials 305 305 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…, start BioPython stubs
|
Woops: |
genehack
left a comment
genehack
left a comment
There was a problem hiding this comment.
few comments, no real problems?
| import Bio.SeqIO | ||
| import os | ||
|
|
||
| from augur.errors import AugurError | ||
| from augur.utils import augur | ||
| from importlib.metadata import version as installed_version | ||
| from packaging.version import Version | ||
| from shlex import quote as shquote | ||
| from shutil import which | ||
| from tempfile import NamedTemporaryFile | ||
| from textwrap import dedent | ||
| from typing import Iterator, Iterable, Union | ||
| from typing import Iterable, Iterator, Optional, Union | ||
|
|
||
| import Bio | ||
| from Bio.SeqFeature import SeqFeature, FeatureLocation | ||
| from Bio.SeqRecord import SeqRecord | ||
| from packaging.version import Version | ||
|
|
||
| from augur.errors import AugurError | ||
| from augur.utils import augur |
There was a problem hiding this comment.
I'm not opposed to this import re-ordering, but I don't really understand the scheme being used — in particular, why is from packaging.version import Version located where it is?
There was a problem hiding this comment.
this is isort, it uses 3 groups: things that ship with Python, things that are dependencies, module itself. Packaging doesn't ship, is a dependency and sorts after B.
|
|
||
|
|
||
| def _read_gff(reference, feature_names): | ||
| def _read_gff(reference: str, feature_names: Optional[Union[set[str], list[str]]] = None) -> dict[str, SeqFeature]: |
There was a problem hiding this comment.
Second time seeing Optional[Union[set[str], list[str]]]; is this worth a
StringSetOrList = Optional[Union[set[str], list[str]]]somewhere?
| for feat in gb.features: | ||
| for feature in gb.features: | ||
| feat = feature |
There was a problem hiding this comment.
I would say if you're going to rename the loop variable, rename all the usages too, don't immediately re-assign it back to the former name.
| from io import RawIOBase | ||
| from shlex import quote as shquote | ||
| from .__version__ import __version__ | ||
| from Bio.SeqFeature import CompoundLocation, FeatureLocation, SeqFeature |
There was a problem hiding this comment.
same question as above about the ordering of this import — why not after L14 with the other Bio.* imports?
| # --------------------------------------------------------------------------- # | ||
| # Convenience aliases # | ||
| # --------------------------------------------------------------------------- # |
There was a problem hiding this comment.
Doesn't really seem to be a comment syntax/style that's used in this codebase, consider aligning or converting to docstrings.
| # internal helpers (return shifted / flipped copies) --------------------- # | ||
| def _shift(self, offset: int) -> "Location": ... | ||
| def _flip(self, length: int) -> "Location": ... |
There was a problem hiding this comment.
if they're internal-only helpers, maybe we shouldn't include types? we shouldn't be using them…
Add types to function that can be type checked and had typing code.
Checklist