-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Array.prototype.toLocaleString uses comma space separator instead of just comma #6361
Comments
Per spec this is implementation defined, but: Should change be consistent - very easy fix change to use GetCommaDisplayString instead of GetCommaSpaceDisplayString here: Updating tests that use this may take a little while though - tagging as good first issue. |
Hi, could i work on this? |
@muhdmud, sorry for the late reply - surely you can, let me know if you need any guidance on this. |
@ppenzin yes thank you for the opportunity! I will have a go at it this week |
@ppenzin Can I just submit a WIP pr with that oneliner change, then subsequently work on the tests? I assume the tests that i am suppose to be looking at is at chakracore/test/string concats, please correct me if im wrong |
@muhdmud Sorry for late reply. Yes, that would be great! |
@rhuanjl, after taking a look at #6499, there is one more place the space is coming from: https://github.com/microsoft/ChakraCore/blob/c459fea95931c6736e4decd817f5c4d7ca40efc5/lib/Runtime/Library/JavascriptArray.cpp#L8383 @muhdmud, we are getting the additional space in the failing tests from https://github.com/microsoft/ChakraCore/blob/3daa56768f45594ec3461731e68865323900acf1/lib/Runtime/PlatformAgnostic/Platform/Windows/ArraysUtility.cpp#L15 |
I run following code in ch 1.11.15, the result of ChakraCore is different from other js engines. Though in specification, “string-concatenation” is not defined clearly, other engines’ output is same.
Code:
output of ch:
"Fire, , Wind, Rain"
output of other engines:
"Fire,,Wind,Rain"
ISec Lab
2019.12.21
The text was updated successfully, but these errors were encountered: