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

Fixed parameter to check cached resolved options #1654

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nodify-at
Copy link
Contributor

As mentioned by @mohd-akram : #1642 (comment) we have to use intl instead of locale

@diesieben07
Copy link
Collaborator

Can you please add a test that fails without this and passes with this fixes?

@nodify-at
Copy link
Contributor Author

@diesieben07,

After a deep analysis, I've investigated the change from loc.intl to loc.locale in the supportsFastNumbers function to assess any potential issues.

Findings:

  • Function Logic: The function primarily depends on loc.numberingSystem. If it's set and not 'latn', the function immediately returns false. If it's 'latn', it returns true without further checks.

  • Impact of Change: Since loc.numberingSystem is usually set by parseLocaleString, the function's outcome is determined before considering loc.intl or loc.locale. Therefore, changing from loc.intl to loc.locale doesn't affect the result.

  • Edge Cases: I attempted to create test cases with various locales and numbering systems (e.g., 'ar-EG-u-nu-latn', 'th-TH-u-nu-latn') to find a scenario where the change would cause incorrect behavior. However, due to the function's logic, no such case was found.

Conclusion:

Given that the function's behavior remains consistent and no adverse effects were identified, I believe the change from loc.intl to loc.locale in supportsFastNumbers is acceptable.

@nodify-at
Copy link
Contributor Author

So, if you don't mind, we can close this PR.

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