-
Notifications
You must be signed in to change notification settings - Fork 4
Add diagram direction support (#58) #61
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
Conversation
kurotych
left a comment
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.
Hello @silwol, I'm very grateful for your contribution.
Everything looks good overall. Please check the two comments, and we’ll be good to go.
src/cli.rs
Outdated
| "top-to-bottom", | ||
| "bottom-to-top", | ||
| "left-to-right", | ||
| "right-to-left", |
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.
Let's have short values: tb, bt, lr, rl in cli parameters
src/mermaid_generator.rs
Outdated
| use tinytemplate::{format_unescaped, TinyTemplate}; | ||
|
|
||
| static MERMAID_TEMPLATE: &str = r#"erDiagram | ||
| static MERMAID_TEMPLATE: &str = r#"erDiagram{{ if direction }} |
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.
start if direction from new line, otherwise it produces white-spaces
static MERMAID_TEMPLATE: &str = r#"erDiagram
{{ if direction }}direction {direction}{{ endif }}
{{ for ent in entities}}{ent}{{ endfor }}
{{ for en in enums}}{en}{{ endfor }}
{{ for fk in foreign_keys}}{fk}{{ endfor }}
"#;
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 whitespace was intentional, I thought an indentation is required for mermaid diagrams, maybe I'm wrong.
If I understand correctly, your proposal will create an empty line if no direction is passed in. That's what I tried to avoid with my approach. If this empty line and the "missing" indentation don't matter, we can go ahead with the proposal, which is what I pushed now.
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.
indentation is required for mermaid diagrams,
indentation is not required
your proposal will create an empty line if no direction is passed in. That's what I tried to avoid with my approach.
You are right. It looks better. I will return if statement to the top to avoid empty line in missing parameter case.
|
Thanks a lot for merging! 🚀 |
Thank you for your contribution 👍 |
Not quite sure whether the tinytemplate templates are formatted as expected, I tried to keep the output format identical when the parameter is absent, and just add a single line if it is present. That results in a somewhat strange template format, not sure what your preference is here.
Hope the PR suits in general, please let me know whether the general direction is ok or if you see room for improvement somewhere.