-
Notifications
You must be signed in to change notification settings - Fork 101
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 XML Page template files in Python 3 #320
Conversation
Without the patch the ptest fail like:
The compilation fails when reaching this line: Zope/src/Products/PageTemplates/engine.py Line 99 in 6dcd4c7
|
34db886
to
7a58724
Compare
Does this PR aim to fix #319? |
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.
This mostly looks great but I have one concern.
try: | ||
text = text.decode(encodingFromXMLPreamble(text)) | ||
except UnicodeDecodeError: | ||
pass |
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.
Maybe it would be better to let the exception happen rather than swallowing it and returning bytes which will probably just fail later?
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.
+1
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.
If I remove the try/except the tests from test_sniffer_xml_utf16_be
and test_sniffer_xml_utf16_le
will fail because the regexp in encodingFromXMLPreamble
method will fail to match the encoding.
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.
I share the concern of @davisagli
try: | ||
text = text.decode(encodingFromXMLPreamble(text)) | ||
except UnicodeDecodeError: | ||
pass |
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.
+1
I removed the try/except but I had to add an |
Fixes #319