Fix type hints #132
Fix type hints #132
Conversation
benoit74
force-pushed
the
fix_type_hints
branch
from
0b7cebf to
48847a4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 32 32
Lines 1345 1370 +25
Branches 229 237 +8
=========================================
+ Hits 1345 1370 +25 ☔ View full report in Codecov by Sentry. |
benoit74
force-pushed
the
fix_type_hints
branch
from
48847a4 to
e4a83dd
Compare
src/zimscraperlib/zim/items.py
Outdated
| @@ -106,7 +104,7 @@ def download_for_size(url, on_disk, tmp_dir=None): | |||
| size, _ = stream_file(url.geturl(), fpath=fpath, byte_stream=stream) | |||
| return fpath or stream, size | |||
|
|
|||
| def __init__(self, url: str, **kwargs): | |||
| def __init__(self, url: str, **kwargs: Any): | |||
There was a problem hiding this comment.
I think that all expected (for which we have processing in the class) values should be individual optionnal keyword arguments with they expected type. We can leave Any for the rest
There was a problem hiding this comment.
Same logic would apply to classes inheriting Item
There was a problem hiding this comment.
I can only agree :)
|
Changes done Nota: please do not merge, I will rebase/push-force before merge first since another PR has been merged in the mean time. |
There was a problem hiding this comment.
Looks good to me but we're now storing several variables per object for stuff that might never be set.
Given those Item are created in very huge number and only released at an unknown/uncontrolled point in the future (when libzim is done copying to disk) I wonder if it's a good idea.
|
I don't know ... you are right, but somehow this is the price to pay to have proper typing in the whole class, otherwise we will still rely on Should we give it a try or rollback the change? |
|
By rollbacking the change I mean changing only the method signatures, it is already a step forward. |
|
Yes the method signature is what's most important because that's the public API. Could you use the |
|
I pushed one more commit to "rollback" what is necessary to keep the get_attr approach while still enhancing the public API. I tend to admit I prefer this "middle-ground" approach. |
We might have many Items, some of them long-lived, so it is maybe better to not store many properties with a None value because they are unused and hence keep a lean memory footprint
benoit74
force-pushed
the
fix_type_hints
branch
from
cd25285 to
824504f
Compare
|
Nota: last push is just a rebase on main last commit. |
Fix #109
Fix #112
Rationale
Type hints were too restrictive / incorrect
Changes
Item**kwargs, the type specified must be the one of individual param values, not the whole dictionaryUnion[str, bool, bytes]type hint was replaced byAny, because the goal of this param is precisely to pass any type which could be further usedFileItemandURLItemwere modified as well because they were very close toStaticItemStaticItemin factadd_itemmethod quite few years agoconvert_imagesrcanddstcan be either aPathlib.Pathor anio.BytesIO(maybe eventyping.BinaryIOortyping.IO[bytes], but I did not find a convenient way to test this to confirm that adherence totyping.IO[bytes]contract is sufficient or if we need something additional fromio.BytesIO)**params, the type specified must be the one of individual param values, not the whole dictionary ; based on existing codebase across all scrapers I assumed thatstris sufficient.save_imagehad to be changed as well to reflect changes made inconvert_image