openzim / python-scraperlib

Collection of Python code to re-use across Python-based scrapers
GNU General Public License v3.0
17 stars 16 forks source link

Fix type hints #132

Closed benoit74 closed 4 months ago

benoit74 commented 4 months ago

Fix #109 Fix #112

Rationale

Type hints were too restrictive / incorrect

Changes

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5d95d59) 100.00% compared to head (cd25285) 100.00%.

:exclamation: Current head cd25285 differs from pull request most recent head 824504f. Consider uploading reports for the commit 824504f to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #132 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 32 32 Lines 1345 1370 +25 Branches 229 237 +8 ========================================= + Hits 1345 1370 +25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

benoit74 commented 4 months ago

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.

benoit74 commented 4 months ago

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 get_attr and mostly untyped values.

Should we give it a try or rollback the change?

benoit74 commented 4 months ago

By rollbacking the change I mean changing only the method signatures, it is already a step forward.

rgaudin commented 4 months ago

Yes the method signature is what's most important because that's the public API.

Could you use the getattr way for the optional stuff in an independent commit so we have a better view at both options?

benoit74 commented 4 months ago

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.

benoit74 commented 4 months ago

Nota: last push is just a rebase on main last commit.