style(mypy): Enforcing typing for superset.translations module #9800
Conversation
john-bodley
changed the title
john-bodley
changed the title
Codecov Report
@@ Coverage Diff @@
## master #9800 +/- ##
==========================================
+ Coverage 64.35% 70.85% +6.50%
==========================================
Files 536 588 +52
Lines 29106 30472 +1366
Branches 2806 3153 +347
==========================================
+ Hits 18732 21592 +2860
+ Misses 10194 8767 -1427
+ Partials 180 113 -67
Continue to review full report at Codecov.
|
john-bodley
force-pushed
the
john-bodley--mypy-enforcement-translations
branch
from
7486f28
to
0c8f962
Compare
superset/translations/utils.py
Outdated
|
|
||
| # Global caching for JSON language packs | ||
| ALL_LANGUAGE_PACKS: Dict[str, Dict[Any, Any]] = {"en": {}} | ||
|
|
||
| DIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
||
|
|
||
| def get_language_pack(locale): | ||
| def get_language_pack(locale: str) -> Optional[Dict[Any, Any]]: |
There was a problem hiding this comment.
I believe we could make the key str to improve differentiation slightly without complicating things too much (strictly speaking it seems the structure is Optional[Dict[str, Union[str, Dict[str, Union[Dict[str, str], List[str]]]]]] which quite frankly looks awful and probably won't help detect any errors)
john-bodley
force-pushed
the
john-bodley--mypy-enforcement-translations
branch
from
0c8f962
to
ad0de37
Compare
| @@ -38,7 +38,7 @@ def get_language_pack(locale): | |||
| try: | |||
| with open(filename, encoding="utf8") as f: | |||
| pack = json.load(f) | |||
| ALL_LANGUAGE_PACKS[locale] = pack | |||
| ALL_LANGUAGE_PACKS[locale] = pack or {} | |||
| except Exception: # pylint: disable=broad-except | |||
| # Assuming english, client side falls back on english | |||
There was a problem hiding this comment.
Perhaps we should put:
pack = {}in this block, and move
ALL_LANGUAGE_PACKS[locale] = packoutside of the try/except.
Then we could remove the Optional from the get_language_pack return type
There was a problem hiding this comment.
@etr2460 I thought of that however it would change the return type, i.e., it could no longer be None and per the comment it seems like this could be problematic for the client.
There was a problem hiding this comment.
Doesn't an empty dict resolve to false? so if the locale is English then this returns an empty dict (based on the constant at the top of the file)
There was a problem hiding this comment.
And this seems to indicate that if this isn't set, we default to an empty object on the frontend anyway: https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-translation/src/Translator.ts#L26
There was a problem hiding this comment.
@etr2460 the idea here was to keep the status quo. pack could be None per line 35 and remain that way if an exception is thrown either when opening the file or parsing the JSON.
There was a problem hiding this comment.
I'm still not 100% convinced that it's not worth cleaning up a bit and making this not Optional, but if you don't want to make such a change in the typing PR, that's fine. I can make a follow up PR to update this logic as I think it's cleaner
john-bodley
changed the title
Co-authored-by: John Bodley <john.bodley@airbnb.com>
mistercrunch
added
🏷️ bot
SUMMARY
Adding
mypytype enforcement to thesuperset.translationsmodule.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI.
ADDITIONAL INFORMATION
Reviewers
to: @etr2460 @villebro