Add comprehensive type annotations to util module#2980
Conversation
jonathangreen
force-pushed
the
chore/type-hint-util
branch
from
631033b to
ee9c936
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
==========================================
+ Coverage 92.81% 92.94% +0.13%
==========================================
Files 454 453 -1
Lines 42954 42818 -136
Branches 6000 5973 -27
==========================================
- Hits 39866 39799 -67
+ Misses 2018 1952 -66
+ Partials 1070 1067 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonathangreen
force-pushed
the
chore/type-hint-util
branch
from
5f44575 to
10b2765
Compare
Improve type annotations in util module - Replace Any with specific types (re.Match[str], TextBlob) - Simplify type annotation in OPDSMessage.__init__ - Remove unnecessary defensive checks in regex replacement functions - Remove unused Any imports Replace Any with Contributor type for author parameters - Add TYPE_CHECKING import for Contributor model - Update _wordbags_for_author to use Contributor instead of Any - Update author_name_similarity to use Iterable[Contributor] - Avoids circular import by using TYPE_CHECKING Fix web publication manifest type annotations - Add JSONLDContext type alias for JSON-LD @context values - Allow context parameter to accept str, dict, or list of str/dict - Allow href parameters to accept None (filtered by _append method) - Add explicit type annotation to context_with_extension in FindawayManifest - Fixes mypy errors without using generic Any types
jonathangreen
force-pushed
the
chore/type-hint-util
branch
from
10b2765 to
9ad74be
Compare
| similarity = self.similarity_to(candidate) | ||
| if similarity >= threshold: | ||
| yield candidate | ||
|
|
There was a problem hiding this comment.
Removed because these functions were unused.
| from palace.manager.sqlalchemy.model.contributor import Contributor | ||
|
|
||
|
|
||
| class MetadataSimilarity: |
There was a problem hiding this comment.
Big chunks of this class were removed because they were unused, except for in tests.
| # The main title and the full title were the same. | ||
| return None | ||
| return subtitle | ||
|
|
| def __init__(self, bigrams: Counter[str]) -> None: | ||
| self.bigrams = bigrams | ||
| self.proportional = Counter() | ||
| self.proportional: dict[str, float] = {} |
There was a problem hiding this comment.
Using dict instead of Counter because Counter is typed for int values, and we need to store float scores. It does appear to work at runtime, but I'm not sure its actually intentional behavior. Using dict + sorted() is cleaner than adding multiple type ignores for Counter.most_common().
| } | ||
| english_bigrams = Bigrams(Counter()) | ||
| english_bigrams.proportional = Counter(english_bigram_frequencies) | ||
| english_bigrams.proportional = dict(english_bigram_frequencies) |
There was a problem hiding this comment.
Same counter float issue here as https://github.com/ThePalaceProject/circulation/pull/2980/changes#r2683624940
| """A helper object for creating etree elements.""" | ||
|
|
||
| def __dict__(self): | ||
| def __getstate__(self) -> dict[str, Any]: |
There was a problem hiding this comment.
This is old code, and it rightly fails type checking because this isn't how __dict__ is defined. Since it mentions pickling, it seems like __getstate__ was actually the intended override here. 🤷🏻
| """ | ||
| if not match or len(match.groups()) < 1: | ||
| return match | ||
|
|
There was a problem hiding this comment.
This method must return a str, so this defensive code is incorrect. It seems to be un-needed where it us used. We could also modify this to raise instead, although in that case match.groups()[0] + "MD" will raise. So I think we are okay to drop this defensive code
| """ | ||
| if not match or len(match.groups()) < 1: | ||
| return match | ||
|
|
| """ | ||
| if not match or len(match.groups()) < 3: | ||
| return match | ||
|
|
| def best_choices(self, n: int = 3) -> list[tuple[str, float]]: | ||
| """Choose the best `n` choices among the current summaries.""" | ||
| scores = Counter() | ||
| scores: dict[str, float] = {} |
There was a problem hiding this comment.
- Remove empty TYPE_CHECKING block from util/__init__.py - Change AtomFeed.__str__() to return empty string instead of "None" when feed is None
jonathangreen
changed the title
|
Codecov is complaining about patch coverage, but the coverage gaps were pre-existing, so I think we can ignore it on this PR. |
2 participants
Description
This PR adds comprehensive type hints to the entire
utilmodule and removes these modules from the mypy ignore list. The changes improve code quality and type safety while discovering and fixing several bugs. In the process of type hinting, I found several unused sections of code that were removed.Motivation and Context
The util module lacked type hints, making it harder to catch type-related bugs and reducing IDE support. Adding type hints:
pyproject.tomlBugs Fixed
flask_util.py- Fixed dangerous mutable defaultheaders={}permanent_work_id.py- Changedselftoclsis Noneinstead of== None__dict__method inopds_writer.py- Renamed to__getstate__and fixed.items()callType Improvements
Anytypes with specific types (re.Match[str],TextBlob,Contributor)Counter[str],dict[str, float],list[str]TYPE_CHECKINGpattern to avoid circular importsHow Has This Been Tested?
Checklist