(many) Make number-to-string conversions use invariant culture #261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
perlun
added
bug
perlun
commented
Jan 24, 2022
src/Perlang.Common/Utils.cs
Outdated
| else if (@object is IConvertible convertible) | ||
| { | ||
| // The explicit IFormatProvider is required to ensure we use 123.45 format, regardless of | ||
| // host OS language/region settings. See #xxx for more details. |
There was a problem hiding this comment.
TODO: create a proper issue for this and amend these comments, pointing the reader to the issue in question.
perlun
force-pushed
the
fix/make-tests-work-on-non-US-cultures
branch
from
b3ec654
to
e385c9c
Compare
|
(Temporarily cherry-picked the changes from #262 into this branch to ensure that it runs cleanly before merging. We'll need to merge this first and then the other one, because this fixes the underlying problems that the CI PR highlights.) |
CI is clean. Will fix some remaining issues in this and get it merged, so we can get #262 to also be mergeable (CI clean). |
perlun
force-pushed
the
fix/make-tests-work-on-non-US-cultures
branch
from
e385c9c
to
261cf33
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I discovered these errors by accident, when trying to run the test suite on Windows (for an entirely different purpose 馃檪). We had some assumptions in the unit tests which failed because the
ToString()conversions in .NET uses theCultureInfo.CurrentCultureby default. This means that if you have your Windows running with e.g. Swedish regional settings (which is my setup), adoublevariable with a value of123.45will be rendered as123,45.While this is preferable in some contexts, I think we'll go with the "safe and consistent" approach for now, if not for any other reason that the unit/integration tests currently depend on these semantics. It doesn't provide a good dev experience for people trying this out on Windows/with their machine configured to use a different locale, if they would get a bunch of silly errors.