-
Notifications
You must be signed in to change notification settings - Fork 2k
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
PUBDEV-8460 fix _upload_python_object method #5999
Conversation
7653e9d
to
878d837
Compare
ec2/h2o-cluster-launch-instances.py
Outdated
fpublic = open(publicFileName, 'w', encoding='utf-8') | ||
fprivate = open(privateFileName, 'w', encoding='utf-8') |
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.
probably better, but not sure it belongs here
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.
@@ -72,7 +72,7 @@ def main(): | |||
# magic_files = {} | |||
for filename in locate_files(ROOT_DIR): | |||
print("Processing %s" % filename) | |||
with open(filename, "rt") as f: | |||
with open(filename, "rt", encoding='utf-8') as f: |
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'd rather delete that entire file: unused, unclear about what its original intent (read some magoc comments and then what?), and no example of "magic comment" in h2o-py
or h2o-bindings
.
Thanks for the discovery :)
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.
yes, I haven't found any example of usage of this.. @michalkurka do you know what this could be useful for?
h2o-py/tests/testdir_parser/pyunit_enforce_utf-8_in_open_method.py
Outdated
Show resolved
Hide resolved
h2o-py/tests/testdir_parser/pyunit_enforce_utf-8_in_open_method.py
Outdated
Show resolved
Hide resolved
h2o-py/h2o/frame.py
Outdated
@@ -139,7 +140,7 @@ def _upload_python_object(self, python_obj, destination_frame=None, header=0, se | |||
|
|||
# create a temporary file that will be written to | |||
tmp_handle, tmp_path = tempfile.mkstemp(suffix=".csv") | |||
tmp_file = os.fdopen(tmp_handle, 'w') | |||
tmp_file = os.fdopen(tmp_handle, 'w', encoding='utf-8') if sys.version_info >= (3, 5) else os.fdopen(tmp_handle, 'w') |
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.
as we apparently have to stick with os.fdopen
here due to Py2 encoding inconsistencies, please use PY2
constant instead of sys.version
as we should always do: much easier to search for usages then, we don't support Py3 < 3.5 anyway.
maybe a simpler way to avoid redundancies:
# as a global var
__fdopen_kwargs = {} if PY2 else {'encoding': 'utf-8'}
# each time we need fdopen:
tmp_file = os.fdopen(tmp_handle, 'w', **__fdopen_kwargs)
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.
hopefully we don't write much on the filesystem, as the write support for strings in Py2 is a total disaster as it doesn't automatically convert str
to proper strings (ie. unicode
), hence this need to use fdopen
apparently here as it seems to write without any check/transformation (returned file object is not a TextIOWrapper
in Py2).
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.
improved in 25e4ce9
the test is still failing because, in short,
in dev-python-3.7 also
There are these locales:
so in h2o_env_python3.7 I will 'reset' manualy to C.UTF-8 |
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.
LGTM, thanks @koniecsveta
a7e518e
to
ec14aff
Compare
ec14aff
to
cd9bd87
Compare
for py 3.5 and higher, the issue is fixed by adding parameter
encoding='utf-8'
toos.fdopen()
callfor py 2.7,
os.fdopen()
doesn't support parameterencoding
, but the issue is anyway not reproducible, by the same test that it is for py 3.8