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.

Sign up for GitHub

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

Jump to bottom

Remove CultureInfo from PowerFxConfig #990

Closed
MikeStall opened this issue Jan 11, 2023 · 2 comments
Closed

Remove CultureInfo from PowerFxConfig #990

MikeStall opened this issue Jan 11, 2023 · 2 comments
Assignees
@LucGenetier
Labels
Breaking API
Milestone
GA 1.0

Comments

@MikeStall
Copy link
Contributor

Removing the config from PowerFxConfig.

See #918, and discussion in #872 .

We have 3 uses of culture:

  • Parser mode
  • errors
  • runtime culture

These can often be different values.
Setting this once in config may seem convenient, but it becomes a problem when we need to set different values.

we should avoid a global setting on config, and just rely on the caller being explicit.

Related - we should never be getting this from thread's current culture. Again, require the caller to be explicit.
@MikeStall MikeStall added this to the GA 1.0 milestone Jan 11, 2023
@MikeStall MikeStall mentioned this issue Jan 30, 2023
Merged
MikeStall added a commit that referenced this issue Jan 30, 2023
@MikeStall
CultureInfo support for errors (#1040)
6773f60 
Allow Errors to be in separate culture info that parser. 
Fix #918, and also #1039. Sets up a fix for #990.  

Make ExpressionError.Message localize lazily.  
- Callers can get the localized message in any culture after the
parse/bind
- just checking IsSuccess can then avoid localization since that doesn't
read the message property


Add test to ensure we're not using Thread.CurrentCulture when a culture
is provided.
- Update a bunch of usages. StartsWith(), EndsWith() ,etc require
StringComparison.Ordinal, else they will check current culture.
- RegEx required CultureInvariant 


Add more tests
- add ExpressionErrorTests to get 100% code coverage on ExpressionError.
- add MultiCulture* tests to demonstrate how we can set
Parse/ErrorMessage/Runtime cultures independently
- remove parserOptions usage in LSP that wasn't being used.
@MikeStall MikeStall self-assigned this Mar 2, 2023
@MikeStall
Copy link
Contributor Author

Different usages for culture: Parser / Runtime / Error
All can be set independently.
Parser - via ParserOptions
Error
Runtime - via ServiceProvider

Config.Culture is a problem because which culture does it refer to?

May need to be updated:
#872

See ParseAndErrorLocaleAreDifferent test.

These have culture via ParserOptions:
Engine
CheckResult

So any instance method on these already has culture and doesn't need a new param.

TBD - IntellisenseData?

CheckWrapper is UDF stuff; just ignore it.

@MikeStall MikeStall assigned LucGenetier and unassigned MikeStall Mar 28, 2023
@LucGenetier LucGenetier linked a pull request Mar 29, 2023 that will close this issue
Remove PowerFxConfig.Culture parameter #1272
Closed
@MikeStall
Copy link
Contributor Author

See #1292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LucGenetier LucGenetier

Labels
Breaking API
Projects
None yet
Milestone
GA 1.0
Development

Successfully merging a pull request may close this issue.

Remove PowerFxConfig.Culture parameter microsoft/Power-Fx
2 participants
@MikeStall @LucGenetier