Skip to content
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

ENH: Add utf-8 support for savetxt #8644

Closed
wants to merge 1 commit into from

Conversation

DavidToca
Copy link

@@ -26,17 +26,17 @@

def asunicode(s):
if isinstance(s, bytes):
return s.decode('latin1')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should not be touched. Any changes should be restricted to npyio.py.

@pv
Copy link
Member

pv commented Feb 19, 2017

I don't think hardcoding an encoding like this is the correct way forward.

There are a number of things to address in savetxt/loadtxt, and the strategy should be explicitly discussed to be sure to get it right:

  • The savetxt handling of byte string formatting is buggy on Py3 --- this traces back to % formatting being not supported for bytes on Python < 3.5.
  • Writing to text streams vs. byte streams.
  • Selecting the encoding to use for cases where conversions are necessary.

You also need to add corresponding tests to the test suite for a PR to be accepted.

@DavidToca
Copy link
Author

DavidToca commented Feb 19, 2017

@pv thanks for your review, so, should I add this as an issue, or post it in on the dev mailing list in order to be discussed?

@pv
Copy link
Member

pv commented Feb 19, 2017

What I mean by "explicitly discussed" is that I think it would be useful to first write down an overview/spec of how you think it should work, for example in the pull request description above. For example, what should happen with dtype='S and type='U' strings etc., how to select an encoding, whether there should be a possibility to write/read text streams instead of just byte streams --- once there's such an overview/spec of the intended behavior, it's easier to check whether the code does what was intended, and whether what is intended is consistent.

@juliantaylor
Copy link
Contributor

savetxt has an encoding option in 1.14 so this PR can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants