-
Notifications
You must be signed in to change notification settings - Fork 1
add key representation and mermaid ER diagrams support #1
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you for contributing! I have these features in mind, so I'm glad to see that this piqued your interest.
Part of this is on me to add CONTRIBUTING.md
which is still missing from the repository, but in general please start with an issue first so we can discuss the implementation path before committing your time on this — I want you to have a good time contributing, knowing that when you write the code it will very likely get approved.
For a path forward, would you mind moving the mermaid bits into a separate PR so we can discuss the two separate features in isolation? I think there is value in having Mermaid diagrams, but I'm not sure if I'm ready to commit on the implementation details yet (e.g. should it be in the same file? separate? should we show every field or just the table name with their foreign keys?) I'm leaning towards going minimalistic on these, but I can be convinced both ways 😄
What's your timeline for working on this? I'm trying to gauge whether I can set up golden tests before this work is finalized. Edit: sample golden test is out #2.
Once again, I appreciate the interest and initiative, thank you! Cheers from California.
PK bool | ||
FK string | ||
FK_ref string | ||
UK bool |
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 went back and forth on this one in my head, and I think it might be better for us to keep these in separate struct as TableIndex
? That should allow us to be expansive too with informing the index kind (e.g. btree
).
I find that making the view as close as possible to \d
output would be best as it doesn't burden users with re-familiarizing on output type. What do you think?
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.
Sorry not to have replied sooner. I have little time to contribute for this project (and little free time in general).
You're right, I would normally have created two issues to discuss and 2 PR for this contrib, I just wanted to give some views what could be done to my mind and prototype it. I didn't give more "context" to my contrib.
The story:
My main goal was to prototype constraints on tables such as primary key and foreign keys. Then I just remembered of what schemaspy did : a dot generated ERD. Then I thought about contextualized diagrams with mermaid. And at that point, as I already prototyped it, I decided to submit my work as DRAFT, not to be integrated into main code but to give some ideas (you also had them, so that's fine).
Maybe textual information is fine just like PostgreSQL psql \d
but having (markdown) tables with PK and FK seems more visual to me.
Refering tables+column could be displayed in psql way.
Moreover, my main goal would have been to show constraints on columns : references, not nul, checks and so on.
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.
Depending on my free time, I could review this work to be more consistent. But if you want to go faster, feel free to get inspiration, discard my PR or reuse parts (and I'm totally fine with it). If you prefer let me continue this work, you have to know about my little time availability and it may require me weeks to finish (sorry about that).
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.
No need to apologize — I'm glad that someone else resonated with the usefulness with this project! I'm probably going to spend some time to make some progress on the project in a few weeks time, but I will surely let you know if I decide to tackle these features specifically.
Thank you!
columns, err := m.db.ListColumns(ctx, schema, tableName) | ||
if err != nil { | ||
return fmt.Errorf("fetching columns for '%s.%s': %w", schema, tableName, err) | ||
} | ||
for _, column := range columns { | ||
for idx := range columns { |
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.
was it necessary to use idx
instead of getting the column
struct directly?
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.
The main reason is that I change slice values later in constraints checks. Using column is using a copy of the slice item. Maybe I'm wrong as I'm less acquainted with Golang than with other languages. It looked like it did not work without slice indirection.
Draft: This is a first attempt to put PK , FK an UK (using information_schema rather than pg_catalog) and referred columns.
This patch also draws basic mermaid.js Entity Relationship (not good for edges).
Add configuration switch to enable/disable mermaid generation.