-
Notifications
You must be signed in to change notification settings - Fork 61
fix: Apply CultureInfo.InvariantCulture not to be affected by system locale setting
#386
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
fix: Apply CultureInfo.InvariantCulture not to be affected by system locale setting
#386
Conversation
CultureInfo.InvariantCulture not to be affected by locale settingCultureInfo.InvariantCulture not to be affected by locale setting
CultureInfo.InvariantCulture not to be affected by locale settingCultureInfo.InvariantCulture not to be affected by system locale setting
4772f5c to
ea0c04e
Compare
e09c05e to
fd86716
Compare
bfff522 to
bf7bd64
Compare
DaveSkender
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but check the Codacy findings to fix, or ignore any false-positives.
@DaveSkender Yeap. All of those can be ignored |
|
@DaveSkender Just a trivial thing. Which version should be increased? minor or patch? |
If there’s no change to the actual API and is not a breaking change, patch is all you’d need. If you consider this a new feature, minor. Just a patch IMO, even if a big relief to users. |
|
I don't have any open concerns with this, LGTM. Please proceed with it. |
|
@DaveSkender An idea came up with!
stock-indicators-python/tests/common/test_cstype_conversion.py Lines 36 to 42 in c9a40d8
|
|
@DaveSkender I had a quick test. And this way has max 3x performance gain! if cs_decimal is not None:
return CsDecimal.ToDouble(cs_decimal)But few tests fails due to precision problem |
Description
Fixes:
decimal.InvalidOperationwith small numbers #385Checklist