[GitHub] [thrift] aaronmjones commented on pull request #2401: THRIFT-3840: C++ TJSONProtocol still using locale dependent formatting

[GitBox <gi...@apache.org>]

aaronmjones commented on pull request #2401:
URL: https://github.com/apache/thrift/pull/2401#issuecomment-855263748


   > Would it be possible to add also a small test directly to `lib/cpp/test/ToStringTest.cpp` to see more transparently what case will fail? Is this only about JSON or are all number converters affected (I assume the latter is true)?
   
   Yeah, ToStringTest.cpp looks like a better place for this than JSONProtoTest.cpp. The problem is about number formatting and not JSON in particular. I'll add unit tests for integer and float string formatting to ToStringTest.cpp and remove the changes to JSONProtocolTest. The integer string formatting test will set the global locale to "en_US.UTF-8" which uses comma for a thousands separator. The float string formatting test will set the global locale to "de_DE.UTF-8" which uses a decimal comma -- not a decimal point.
   
   Before changes to TToString.h, the tests would fail like:
   
   ```
   # ./UnitTests 
   Running 57 test cases...
   /thrift/src/lib/cpp/test/ToStringTest.cpp(52): error: in "ToStringTest/locale_en_US_int_to_string": check to_string(1000000) == "1000000" has failed [1,000,000 != 1000000]
   /thrift/src/lib/cpp/test/ToStringTest.cpp(62): error: in "ToStringTest/locale_de_DE_float_to_string": check to_string(1.2) == "1.2" has failed [1,2 != 1.2]
   ```
   
   I'll have the changes ready soon.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org