-
Notifications
You must be signed in to change notification settings - Fork 193
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 broken support to py2.7 #256 #257
base: master
Are you sure you want to change the base?
Conversation
@@ -10,6 +10,13 @@ | |||
|
|||
log = logging.getLogger(__name__) | |||
|
|||
# Py2 vs Py3 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.
Can you use six
for the python version checking? eg
import six
if six.PY3:
# ...
else:
# ...
or even
_encoding = 'utf-8' if six.PY3 else str
But looking at the documentation it looks like the most portable solution would be to pass
record["xml"] = etree.tostring(mdtree, pretty_print=True, encoding='utf-8')
regardless of the Python version. Does this work as expected on Python 2.7 (and 3)?
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.
It was 'unicode' and have been change into 'str' by:
e063522#diff-db75a495bf76416216c3b4fe8cc1038bc53d22a19cb0990cbd823b17c2d67364R104
Now I can't be sure about py3 (don't have a working environment) but you could be right.
To avoid causing problem I proposed encoding='utf-8' for py2 and 'as it is' (futurized) for py3.
Can someone confirm about py3?
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.
It seems ok to me.
There are tests now, so such change could be made, and we see how the CI tests go :)
@ccancellieri The aim is to support Python 2 for versions of CKAN <= 2.9, and the tests are run against these versions (see eg https://github.com/ckan/ckanext-spatial/actions/runs/1394064993). If there is a Python 2 failure it means that test coverage is lacking there and something slipped through the cracks. |
This may fix #256