search

python-babel / babel

The official repository for Babel, the Python Internationalization Library
http://babel.pocoo.org/
BSD 3-Clause "New" or "Revised" License
1.32k stars 440 forks source link

Make LocaleDataDict Generic #961

Open DenverCoder1 opened 1 year ago

DenverCoder1 commented 1 year ago

Type annotation improvements:

Other changes moved to #966

codecov[bot] commented 1 year ago

Codecov Report

Merging #961 (96a7a52) into master (373a52f) will decrease coverage by 0.02%. The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
- Coverage   90.91%   90.90%   -0.02%     
==========================================
  Files          25       25              
  Lines        4350     4353       +3     
==========================================
+ Hits         3955     3957       +2     
- Misses        395      396       +1
Impacted Files Coverage Δ
babel/localedata.py 95.16% <94.73%> (-0.71%) :arrow_down:
babel/core.py 96.52% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

akx commented 1 year ago

2. Changed Locale.parse to always return a Locale (this fixes type issues in multiple places where the return type is assumed to not be None)

That's a great change – I don't think it messes up any valid downstream workflows. However seeing as that retval was added here https://github.com/python-babel/babel/commit/8b7c5e1585b52ea110a5e884151d4eaa2175985d in a commit that talks about likely subtag resolving, could you make sure that it doesn't affect that functionality? I think we might need to do if identifier is None: raise IdentifierNone() and catch it where there currently are other if locale is not Nones...

DenverCoder1 commented 1 year ago
  1. Changed Locale.parse to always return a Locale (this fixes type issues in multiple places where the return type is assumed to not be None)

That's a great change – I don't think it messes up any valid downstream workflows. However seeing as that retval was added here 8b7c5e1 in a commit that talks about likely subtag resolving, could you make sure that it doesn't affect that functionality? I think we might need to do if identifier is None: raise IdentifierNone() and catch it where there currently are other if locale is not Nones...

As far as I can tell, there aren't any locations in the lib that expect the return type of Locale.parse to be anything other than Locale. I don't see any places where parse is used and then the type is checked to be None or not be None.

The places that check for locale being None are ones where a different method is used.

It could raise a different exception from the standard ValueError, but it doesn't seem that it would need to be caught internally.