search

openzim / python-scraperlib

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

Process / return a new typed Lang class in i18n methods #183

Closed benoit74 closed 3 months ago

benoit74 commented 3 months ago

Fix #151

Changes:

codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 100.00%. Comparing base (0573638) to head (ac9249c).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #183 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 33 33 Lines 1603 1640 +37 Branches 291 306 +15 ========================================= + Hits 1603 1640 +37 ```

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

benoit74 commented 3 months ago

The dict-ness is just the least effort path. Code is regularly updating / merging multiple properties at once, and it is a mess / very verbose to re-implement without the dict facilities (see e.g. update_with_macro with I don't know how to reimplement with a proper type). Since the main goal was to expose something more useful rather than a problem of maintainability, I considered that dict-ness was the appropriate solution.

That being that, I would also prefer to use a proper type if this does not makes the code too verbose. Shall we use pydantic BaseModel as base class? It would be a new dependency, it would help a bit, even if it would probably not solve all issues

benoit74 commented 3 months ago

(and using a proper type was also my initial trial, but I quickly realized it was not that convenient to write)

rgaudin commented 3 months ago

Well I think you did not read my comment to the end and that none of your arguments make sense but given it was late when you replied, I think fresh eyes will have us agree that what I commited is simpler and has no downsides. Revert if that's not the case. I only tested via running the unit tests

benoit74 commented 3 months ago

I did read it, just I'm not that attached to dict, and as said I would have preferred to use a proper type that does not subclass dict.

Let's go with dict, I will learn how to do it with a proper type next time (and maybe it is just not "wishable"), I like your simplification anyway.

I'll just replace dict by Dict to prepare for the future.

benoit74 commented 3 months ago

Edit: I'll just replace dict by Dict to prepare for the future dict is already the future ^^