terrajobst / nquery-vnext

A Roslyn inspired rewrite of NQuery
MIT License
72 stars 16 forks source link

CultureInfo.InvariantCulture in BuiltInFunctions #24

Closed StevenThuriot closed 2 years ago

StevenThuriot commented 5 years ago

Hi,

Currently, the BuiltInFunctions all use CultureInfo.InvariantCulture as their culture when attempting to do the conversion. While this seems fine for e.g. en-US, it does give quite some issues in other parts of the world where a decimal comma is used instead of a dot.

Would it not be better to replace CultureInfo.InvariantCulture with CultureInfo.CurrentCulture so the user can actually specify what kind of conversion they're looking for? Or even just passing around a culture through the Query class?

( It also seems it's not consistent, since CharIndex, Upper, Lower, ... do use the current culture instead of Invariant )

Kind Regards, Steven

dallmair commented 5 years ago

From my point of view, any hard-coded variant will always cause pain for some users.

We're using NQuery in non-US regions with decimal comma, and system setup of these users is accordingly, i.e. CultureInfo.CurrentCulture is de-DE, es-ES, etc., but we use NQuery in information exchange scenarios where things are always in CultureInfo.InvariantCulture format. So the current implementation works fine for us, but a hard switch to CurrentCulture would not.

Some sort of configuration would be best, IMHO. Either as a static property right on the BuiltInFunctions class, or as a config option for both Query and Expression<T> (probably makes most sense to add it to DataContext then?).

StevenThuriot commented 5 years ago

Fair enough! It definitely makes sense not to just change it, as backwards compatibility is still important.

terrajobst commented 5 years ago

I think the built-in functions should default to invariant because you tend to have more bugs when queries behave differently based on locale. That being said, I do think it would be nice to allow overloads to specify the culture. I'll need to think about how we're going to expose this though.

StevenThuriot commented 5 years ago

If you need some help implementing, let me know! I'd love to give a hand.