standardize : allow users to specify output encoding #118
Conversation
There was a problem hiding this comment.
Hi @jbdesbas, thanks for opening this PR and apologies for the delay in reviewing! I've left some nitpicks, but generally I think this is a useful feature. One thing I would like to ask though, is whether you can add a unit test or two that demonstrate the desired behavior. This will help to verify that your change is doing what it's supposed to do. Thanks again!
|
hi @GjjvdBurg , thank you for you review . I agree with your comments 👍 and I'll work on it as soon as possible |
|
Change looks good to me! There's some formatting errors that hopefully should be easy to fix by running black on your code. |
tests/test_unit/test_console.py
Outdated
| # Excel format (i.e. RFC4180) *requires* CRLF | ||
| crlf = "\r\n" | ||
| exp = crlf.join(["Å,B,C", "é,ü,中", "4,5,6", ""]) | ||
| with open(tmpoutname, "r", newline="") as fp: |
There was a problem hiding this comment.
I'm not a 100% sure but I wonder if the unit test is failing on windows because this open call doesn't specify an encoding
|
The code looks good to me but unfortunately the tests are failing on windows. Let me know if you'd like any help investigating |
|
You're right, it makes sense since Windows use non-utf8 as default encoding. |
|
All tests passed! 🎉 Thanks again for contributing @jbdesbas! |
Add
-E --target-encodingargument, so the user can specify an output encoding.If omitted, keep the original encoding.