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

Fixing error with python 3 #14

Closed
wants to merge 13 commits into from
Closed

Conversation

mayanksuman
Copy link

@mayanksuman mayanksuman commented Jul 2, 2019

cStringIO is not available in python 3.x. BytesIO (for ascii based StringIO) and StringIO (for utf-8 strings IO) is available from io library.

Fixes #13

cStringIO is not available in python 3.x.
In python 3.x, BytesIO serve the purpose of ascii based StringIO.
@mayanksuman
Copy link
Author

mayanksuman commented Jul 2, 2019

The functions are working for python 3.7 and sqlalchemy 1.3.5. Can we add tox configuration for them?

@fschulze
Copy link
Owner

fschulze commented Jul 2, 2019

You can add the tox configs in this PR, then we also see right away whether the tests pass.

@mayanksuman
Copy link
Author

mayanksuman commented Sep 22, 2019

You can add the tox configs in this PR, then we also see right away whether the tests pass.

I have added tox config. All tests are passing.

@nhoening
Copy link

Not sure if this is helpful at all, but I was just adapting this code for usage in our project, and had to use BytesIO to make it work. I'm using Python 3.8, sqlalchemy-schemadisplay==1.4.dev0, Pillow==7.0.0, pydot==1.4.1, ImageMagick 6.9.7-4 Q16 x86_64 20170114

@fschulze
Copy link
Owner

@nhoening could you give more details on the issue? This PRs tests seem to work fine (which I didn't notice, thanks for updating @mayanksuman !)

@nhoening
Copy link

Not sure what you need to know. I made this function (based on what I saw in this PR) in my script, and I had to use BytesIO, otherwise I got an error.

This is my function:

def show_image(*args, fb_viewer_command:str, kind:str, **kwargs):
    from io import BytesIO
    from PIL import Image
    print("Creating the pydot graph object...")
    if kind == "uml":
        g = create_uml_graph(*args, **kwargs)
    elif kind == "schema":
        g = create_schema_graph(*args, **kwargs)
    else:
        raise Exception(f"Cannot show image of kind {kind} ...")
    print("Creating PNG stream ...")
    iostream = BytesIO(g.create_png())
    print("Showing image ... (fallback viewer is %s)" % fb_viewer_command)
    Image.open(iostream).show(command=fb_viewer_command)

Here is the error I see if I use StringIO instead of BytesIO:

Traceback (most recent call last):
  File "../Common/src/datamodel/visualize_data_model.py", line 173, in <module>
    create_uml_pic(store=args.store)
  File "../Common/src/datamodel/visualize_data_model.py", line 111, in create_uml_pic
    show_image(mappers, **kwargs, fb_viewer_command=FALLBACK_VIEWER_CMD, kind="uml")
  File "../Common/src/datamodel/visualize_data_model.py", line 125, in show_image
    iostream = StringIO(g.create_png())
TypeError: initial_value must be str or None, not bytes

@abitrolly
Copy link
Contributor

@nhoening have you tried to just use the fork directly?

@nhoening
Copy link

No, I already installed 1.4.dev0 (master) for using restricted tables, didn't have time to pin to something more exotic. It works well for me now. If you guys think it's useful, I'll try it out if I ever have 15 minutes.

@nhoening
Copy link

Referring to @mayanksuman 's opening comment, where he does mention BytesIO, maybe the tests only cover one kind of io, namely utf-8, which require StringIO? In any case, the error happens for me for both functions (schema and uml graphs) - I'm not expert enough to judge why I need BytesIO in my setup.

@nhoening
Copy link

@abitrolly I now tried to call this fork's adapted function directly and I get the same error:

  File "src/datamodel/visualize_data_model.py", line 120, in create_uml_pic
    show_uml_graph(mappers, **kwargs)
  File "/home/nicolas/.local/share/virtualenvs/Common-aEUDK1MJ/lib/python3.8/site-packages/sqlalchemy_schemadisplay.py", line 220, in show_uml_graph
    iostream = StringIO(create_uml_graph(*args, **kwargs).create_png())
TypeError: initial_value must be str or None, not bytes

Making the same change as I did before (changing StringIO to BytesIO) makes it work again.

On my system, pydot.Dot().create_png() returns bytes. The docstring also says this (all create_* functions refer to create's docstring:

Creates and returns a binary image for the graph.

create will write the graph to a temporary dot file in the
encoding specified by `encoding` and process it with the
program given by 'prog' (which defaults to 'twopi'), reading
the binary image output and return it as:

- `str` of bytes in Python 2
- `bytes` in Python 3

I can successfully run pytest, so maybe the tests are not touching this code. I suggest someone writes a test for this function or tests this themselves on their machine.

@abitrolly
Copy link
Contributor

@nhoening tox tests generate the coverage - you can see it in https://travis-ci.org/fschulze/sqlalchemy_schemadisplay/jobs/588254164#L252 You can look into the htmlcov dir in tox folder and see if this code is covered by tests or not.

@nhoening
Copy link

It doesn't seem to me that the current tests are covering the issue I'm raising, see attached screenshot of my local tox session (using sqlalchemy-schemadisplay 1.4.dev0).

Screenshot_2020-03-25_12-33-58

@abitrolly
Copy link
Contributor

@mayanksuman while migrating from Travis to GitHub Actions in #28 and #29 I had to do the same code fixes that you did in this PR. Can you see if there is anything left in this PR or it can be closed? And thanks for the contribution anyway.

@abitrolly
Copy link
Contributor

@nhoening it is better to reticket your issue that is not covered by tests, so that it can be addressed separately.

@nhoening
Copy link

nhoening commented Apr 8, 2022

@abitrolly Just checked - I'm still using my own versions of show_uml_graph and show_schema_graph, where I use BytesIO, not StringIO.

I agree this can be a new ticket once I verified it still isn't working.

Do you guys have a version to test this with already that is later than 1.4dev0?

@abitrolly
Copy link
Contributor

@nhoening I use this pyproject.toml syntax to install specific revisions from repository. But the problem is that PyPI doesn't allow to upload projects with such dependencies.

dependencies = [
  "sqlalchemy-schemadisplay @ https://github.com/fschulze/sqlalchemy_schemadisplay/archive/7c35dd35f07e55b2ed3880d62fcb4507152b7ac7.zip"
]

@Zlopez
Copy link
Collaborator

Zlopez commented Jan 29, 2024

Thanks for the contribution, but I decided to go for the #35 as a fix for this.

@Zlopez Zlopez closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show_schema_graph and show_uml_graph fails on python3.x
5 participants