Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LocaleDataDict Generic #961

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

DenverCoder1
Copy link
Contributor

@DenverCoder1 DenverCoder1 commented Jan 25, 2023

Type annotation improvements:

  • Made LocaleDataDict use TypeVars so that the key and value types can be annotated for each instance
  • Fix type issues related to LocaleDataDict
  • Added repr method to LocaleDataDict

Other changes moved to #966

@DenverCoder1 DenverCoder1 marked this pull request as draft January 25, 2023 23:55
@codecov
Copy link

codecov bot commented Jan 26, 2023

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%) ⬇️
babel/core.py 96.52% <100.00%> (ø)

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

@DenverCoder1 DenverCoder1 marked this pull request as ready for review January 26, 2023 00:22
@akx
Copy link
Member

akx commented Jan 26, 2023

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 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...

@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Jan 26, 2023

  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.

@akx akx self-requested a review February 3, 2023 10:00
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary about the LocaleDataDict changes... The numbers changes look alright though, can you break them out into a separate PR?

babel/localedata.py Outdated Show resolved Hide resolved
@DenverCoder1 DenverCoder1 changed the title Make LocaleDataDict Generic and fix core and numbers types Make LocaleDataDict Generic Feb 3, 2023
@DenverCoder1 DenverCoder1 marked this pull request as draft February 3, 2023 15:55
@DenverCoder1 DenverCoder1 marked this pull request as ready for review February 3, 2023 16:24
@DenverCoder1 DenverCoder1 marked this pull request as ready for review February 24, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants