Move to real classes for i18n classes for proper typing in strict mode #229
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 39 39
Lines 2439 2417 -22
Branches 331 322 -9
=========================================
- Hits 2439 2417 -22 ☔ View full report in Codecov by Sentry. |
- Single `Language` class that takes a query and handles everything - `get_language()` and `get_language_or_none` as goto calls to get it - kept `find_language_names()` and `is_valid_iso_639_3()` but reusing `Language`
There was a problem hiding this comment.
Thank you ; it looks good (but we need the docstrings!).
Unfortunately, looking at this makes me want to change it all. I find the API unintuitive and I dont understand why there's Lang, LangAndDetails and all those functions that looks low-level.
Actually, I've tried to reorganize it into something more user-friendly and will push one commit. You let me know what you think about it. It works (updated the tests) but we might want to reorganize the code as everything happens in a single method…
|
I like the new API. I didn't wanted to totally break the old one because I (badly) assumed it was done on-purpose. But I agree it was a nightmare to read and maintain, and probably also to use. I will ask next time ^^ I add missing tests to have 100% coverage. I propose to not mind about the fact that method is long / complex, I find it way easier to understand what we do (and hence to maintain) than what we had before or what we could have by splitting the code in multiple functions. |
This PR is preparatory work for #212.
It is also a rework of #151 / #183 where I obviously was a bit too lazy.
Since this is ready and "significant" and isolated change, I submit it in a dedicated PR.
I've removed two tests which do not really make sense once we use real classes, and are not possible since we cannot init the class with dict values anymore.
CHANGELOG is not updated since anyway we will have to tackle this separately for 5.0.0
This has been tested locally for proper operation with pyright strict mode.