Add type annotations to edition.py and enable mypy strict checking#2978
Add type annotations to edition.py and enable mypy strict checking#2978jonathangreen merged 3 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2978 +/- ##
==========================================
- Coverage 92.82% 92.81% -0.02%
==========================================
Files 454 454
Lines 42942 42953 +11
Branches 5998 6000 +2
==========================================
+ Hits 39860 39865 +5
- Misses 2014 2018 +4
- Partials 1068 1070 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonathangreen
force-pushed
the
chore/add-type-annotations-edition
branch
from
d2afb10 to
2369419
Compare
| if not self.title: | ||
| return None |
There was a problem hiding this comment.
Added null check for self.title to satisfy mypy. calculate_permanent_work_id_for_title_and_author requires a non-None title parameter. This check is consistent with the existing primary_author null check above.
| if ( | ||
| contributor_data.aliases | ||
| and contributor_data.aliases != contributor.aliases | ||
| and list(contributor_data.aliases) != contributor.aliases |
There was a problem hiding this comment.
Changed to list(contributor_data.aliases) for proper comparison. contributor_data.aliases is a Sequence (could be tuple), while contributor.aliases is a list. Direct comparison between tuple and list would always be False.
| if representation is None: | ||
| return |
There was a problem hiding this comment.
Added early return when resource.representation is None. This provides type narrowing so mypy knows representation is not None in subsequent code. The original code would have raised an AttributeError in this case anyway.
| self.cover_thumbnail_url, | ||
| ) | ||
|
|
||
| def add_contributor(self, name, roles, aliases=None, lc=None, viaf=None, **kwargs): |
There was a problem hiding this comment.
Removed **kwargs which was accepting but silently ignoring extra keyword arguments.
| self, | ||
| name: Contributor | str | None, | ||
| roles: Sequence[str] | str, | ||
| aliases: Sequence[str] | None = None, | ||
| lc: str | None = None, | ||
| viaf: str | None = None, |
There was a problem hiding this comment.
Changed roles to Sequence[str] for flexibility (accepts tuples from Pydantic models). Added None to name type since Contributor.lookup accepts it.
| """Assign a contributor to this Edition.""" | ||
| _db = Session.object_session(self) | ||
| if isinstance(roles, (bytes, str)): | ||
| if isinstance(roles, str): |
There was a problem hiding this comment.
Removed bytes from the isinstance check. Roles should never be bytes - this was likely legacy Python 2 compatibility code.
| title = self.title | ||
| if self.subtitle: | ||
| title += ": " + self.subtitle | ||
| title = (title or "") + ": " + self.subtitle |
There was a problem hiding this comment.
Handle edge case where title is None but subtitle exists. Uses (title or "") to avoid TypeError when concatenating.
| if rep: | ||
| self.cover_thumbnail_url = rep.public_url | ||
| break | ||
| else: | ||
| # No thumbnail was found. If the Edition references a thumbnail, | ||
| # it needs to be removed. | ||
| if self.cover_thumbnail_url: | ||
| self.cover_thumbnail_url = None | ||
| self.cover_thumbnail_url = rep.public_url |
There was a problem hiding this comment.
Removed if rep: check - safe because we're in the else block of if not best_thumbnail.representation, so representation is guaranteed to exist. Also removed the for...else block that cleared cover_thumbnail_url - redundant because we only enter this loop when cover_thumbnail_url is already falsy, and the fallback code at line 934 handles the no-thumbnail case.
| def recursively_equivalent_identifier_ids_query( | ||
| cls, | ||
| identifier_id_column: str, | ||
| identifier_id_column: ColumnElement[Any] | int, |
There was a problem hiding this comment.
Fixed incorrect type annotation. The parameter was typed as str but actually accepts either an integer ID or a SQLAlchemy column expression (e.g., Edition.primary_identifier_id) for use in subqueries.
| [c_orig] = list(edition.contributors) | ||
|
|
||
| c1 = edition.add_contributor("c1", Contributor.Role.AUTHOR, _sort_name="c1") | ||
| c1 = edition.add_contributor("c1", Contributor.Role.AUTHOR) |
There was a problem hiding this comment.
Removed _sort_name="c1" argument. This was being silently swallowed by **kwargs but never actually used - the contributor's sort_name is set through Contributor.lookup, not via this kwarg.
- Add comprehensive type annotations to Edition model methods - Remove edition module from mypy exclusion list in pyproject.toml - Use Sequence types for more flexible parameter acceptance - Fix type annotation for identifier_id_column in Identifier class - Add null check for title in BibliographicData.calculate_permanent_work_id - Remove unused _sort_name kwarg from test (was silently ignored by **kwargs)
jonathangreen
force-pushed
the
chore/add-type-annotations-edition
branch
from
3856ca3 to
bede7d5
Compare
|
Codecov is complaining about patch coverage, but the coverage gaps were pre-existing, so I think we can ignore it on this PR. |
Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>
2 participants
Description
Add comprehensive type annotations to the
Editionmodel inedition.pyand enable mypy strict checking for this module.Motivation and Context
This change improves code quality and type safety by:
Editionclassedition.pyfrom the mypy exclusion list inpyproject.tomlSequencetypes for more flexible parameter acceptance inadd_contributor()identifier_id_columnparameter inIdentifierclassHow Has This Been Tested?
_sort_namekwarg (exposed by removing**kwargs)Checklist