-
Notifications
You must be signed in to change notification settings - Fork 20
fix!: Ensure all open() uses UTF-8 instead of default encoding. #171
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!: Ensure all open() uses UTF-8 instead of default encoding. #171
Conversation
6d53058 to
544a429
Compare
Signed-off-by: Karthik Bekal Pattathana <[email protected]>
544a429 to
37905d2
Compare
|
crowecawcaw
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.
Looks good. Can we also add a test or linter rule that will require the encoding format to be specified for all open() calls? It seems very possible someone will forget it in the future, and we'd be unlikely to notice until we get a weird bug later (like the one you were digging into).
Hmm, interesting. I'll add it in an upcoming PR as we need to get this in ASAP so that we can solve customer's issue. |
…bDescription#171) Signed-off-by: Karthik Bekal Pattathana <[email protected]> Signed-off-by: Justin Blagden <[email protected]>



What was the problem/requirement? (What/Why)
Cinema 4D submissions fail to run if filenames contain non-ASCII values. For example, a scene file named
TestSceneñ.c4dwould not render.What was the solution? (How)
While loading the JSON/YAML files, the default
open()uses system's encoding standard which is not UTF-8.This causes issues in other DCCs when they try to read the data.
To ensure that we fix all the issues with encoding I also updated all the
open()calls to also use UTF-8. Kudos to @mwiebe for pointing that out.There was also an issue where the logger used to print out few errors unable to convert non-ASCII characters.
To fix this, I added
sys.stdout.reconfigure(encoding="utf-8")to also force stdout to use UTF-8.What is the impact of this change?
This fixes the issues with non-ASCII characters.
How was this change tested?
While renders now work for characters like
They still fail for emoji and some mathematical operators. Cinema 4D's load document function returns None without details on why it fails.
Yes
Was this change documented?
Yes.
Is this a breaking change?
I discussed this with @AWS-Samuel and considering the packages are public, we have to operate under the assumption that customers are using the packages in their own, potentially private integrations. So even if we handle the change gracefully in all our integrations its still a breaking change.
If so, then please describe the changes that users of this package must make to update their scripts, or Python applications.
Users of this package must update their dependent packages to ensure that they use "UTF-8" encoding as well.
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.