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

Adding support for sqlalchemy 2.x #35

Merged
merged 5 commits into from
Jan 29, 2024
Merged

Conversation

JPchico
Copy link

@JPchico JPchico commented Feb 26, 2023

Due to the changes on how MetaData work in sqlalchemy-2.x one needs to modify the functions to create the diagrams to make sure that the engine information is passed.

The github workflows have also been updated so that up to date actions are used.

The package installation has been moved to pyproject.toml to make it work with flit building tool.

A pre-commit-config.yaml file was also added to ensure that code formatting is more easily maintained.

@naturedamends
Copy link

naturedamends commented Jun 26, 2023

Working for me

pip install git+https://github.com/JPchico/[email protected]

confirmed with

    @app.cli.command("erd")
    @click.argument("file_name")
    def create_erd(file_name):
        from sqlalchemy_schemadisplay import create_schema_graph
        graph = create_schema_graph(
            engine=db.engine,
            metadata=db.metadata,
            show_datatypes=False, # The image would get nasty big if we'd show the datatypes
            show_indexes=False, # ditto for indexes
            rankdir='LR', # From left to right (instead of top to bottom)
            concentrate=False # Don't try to join the relation lines together
        )
        graph.write_png(file_name) # write out the file

@Zlopez
Copy link
Collaborator

Zlopez commented Jan 23, 2024

This PR looks like a complete rewrite to me, but I like how it looks. If the tests are passing I don't see any reason to not merge it.

@Zlopez
Copy link
Collaborator

Zlopez commented Jan 23, 2024

All the tests are passing (tested locally), I'm for merging this one. @RagingRoosevelt What do you think?

@abitrolly
Copy link
Contributor

That's a huge commit. :D @Zlopez I just hope you gave it a good security review.

@Zlopez
Copy link
Collaborator

Zlopez commented Jan 29, 2024

@abitrolly I spend a whole day looking at it. It actually doesn't change much, just makes it better structured.

@abitrolly
Copy link
Contributor

Awesome. Because I am bankrupt on Open Source code reviews. :D

For me having a single file dependency was easier to vendor in https://github.com/abitrolly/sqlite2png/

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.

4 participants