clawpack / clawutil

General utility programs
BSD 3-Clause "New" or "Revised" License
10 stars 31 forks source link

WIP: Use of StringIO in claw_git_status.py #110

Closed rjleveque closed 7 years ago

rjleveque commented 7 years ago

I think @ketch modified src/python/clawutil/claw_git_status.py to change how StringIO is imported from simply

import StringIO

to:

try:
    from StringIO import StringIO
except ImportError:
    from io import StringIO

I fixed one problem in my commit -- if the first import (in the try block) works then StringIO is the function, not the module.

But there seems to also be a problem if the second import statement is used. Forcing it to import from io gives this error:

  File "/Users/rjl/git/clawpack/clawutil/src/python/clawutil/claw_git_status.py", line 77, in repository_status
    output.write("\n\n===========\n%s\n===========\n" % repository)
TypeError: unicode argument expected, got 'str'

even though the docstring for this function says Write string to file.

What was the purpose of this change?

ketch commented 7 years ago

Thanks for catching my mistake!

This change was necessary for Python 3. In Python 3, StringIO is in the io module. The second import statement should only be used in Python 3. The switch from 2 to 3 certainly changes how string encoding is handled, which is probably the cause of the new error. I haven't fully figured out this part, but I think we may instead want to use io.bytesIO; see

http://stackoverflow.com/questions/6479100/python-stringio-replacement-that-works-with-bytes-instead-of-strings

rjleveque commented 7 years ago

Using io.bytesIO in place of StringIO seems to work in Python2 but gave an error in Python3,

    output.write("\n\n===========\n%s\n===========\n" % repository)
TypeError: a bytes-like object is required, not 'str'
rjleveque commented 7 years ago

I notice in JSAnimation it's done this way:

if sys.version_info < (3, 0):
    from cStringIO import StringIO as InMemory
else:
    from io import BytesIO as InMemory

See https://github.com/jakevdp/JSAnimation/blob/master/JSAnimation/html_writer.py

ketch commented 7 years ago

Okay, using what's in JSAnimation works if you also change the problematic lines by encoding the strings, e.g.:

output.write(("\n\n===========\n%s\n===========\n" % repository).encode())

I haven't been able to find a cleaner solution.

ketch commented 7 years ago

Nevermind my last comment; I believe https://github.com/clawpack/clawutil/pull/112 is a neater solution since we mostly are writing strings. This way we just need to convert bytes to strings on two lines.

rjleveque commented 7 years ago

But should we also use the JSAnimation approach of making it clearer that one version is for Python 2 and the other for Python 3? E.g...

if sys.version_info < (3, 0):
    from StringIO import StringIO
else:
    from io import StringIO

rather than a try-except block that's mystifying? (Or at least add some comments.)

mandli commented 7 years ago

The version in JSAnimation is the preferred way to check for versions.

ketch commented 7 years ago

Oh, I agree. It looks like @mandli already merged my PR, so please just issue another with this correction.

rjleveque commented 7 years ago

Well that didn't work... with Python3 it gives things like

===========
classic
===========
/Users/rjl/git/clawpack/classic

b''b''

rather than the desired

===========
classic
===========
/Users/rjl/git/clawpack/classic

--- last commit ---
6c20e2f Merge pull request #78 from rjleveque/setplot_fixes

--- branch and status ---
## master...origin/master

I'll fiddle with it some more...

ketch commented 7 years ago

@rjleveque Are you trying to use BytesIO? I wouldn't use that here; I thought the idea was just to replace the try/except with a version number check (and use StringIO in both cases).

ketch commented 7 years ago

Oh, you mean my PR didn't work! Ack, I didn't check the output...

ketch commented 7 years ago

Now I've checked and it seems to work fine for me.

rjleveque commented 7 years ago

This PR was replaced by #112 and #113.