Add support to change the current CulutureInfo #187
Conversation
There was a problem hiding this comment.
I'm sorry but I can not approve this, at least as-is. CultureInfo manupulation is risky and very open-ended. One can ask for value encapsulation like filters=field==((en-en)data), field2!=currentCulturedData, or full query encapsulation like filters=field==data, field2!=data2&culture=en-en or a global sieve startup parameter. We should consider this widely and then we should implement such support.
By the way, there is a PR (#79) opened about almost 4 years ago. We did resolved the conflicts with another one (#125) and asked for some unit tests. Maybe you can keep on going on that path? What'd you say? 😄
Sieve/Models/SieveOptions.cs
Outdated
| @@ -13,5 +15,7 @@ public class SieveOptions | |||
| public bool IgnoreNullsOnNotEqual { get; set; } = true; | |||
|
|
|||
| public bool DisableNullableTypeExpressionForSorting { get; set; } = false; | |||
|
|
|||
| public CultureInfo CultureInfo { get; set; } = CultureInfo.CurrentCulture; | |||
There was a problem hiding this comment.
It's better to have this setting to be a string which can be read from appsettings.json.
There was a problem hiding this comment.
You are right. Fixed it with solution from #125
Sieve/Services/SieveProcessor.cs
Outdated
| var currentCultureInfo = CultureInfo.CurrentCulture; | ||
| CultureInfo.CurrentCulture = Options.Value.CultureInfo; |
There was a problem hiding this comment.
Errm, no, please.. You should not alter the current culture!..
There was a problem hiding this comment.
You're right. Really bad :-) Also fixed with solution from #125
Hi, I did what you proposed. So now this PR includes the solution from #125 extended with test cases |
When the value of a filter like "MeanRating==1.2" is parsed/converted and the current CultureInfo is e.g. de-DE, then the conversion fails with an exception, because 1.2 is not a valid format in culture.
So it would be good, to let the developer decide which CultureInfo should be used by Sieve.
Therefore this pull request added a CultureInfo property to the SieveOptions, that is used in the Apply method of the SieveProcessor to change the current culture during processing.