-
Notifications
You must be signed in to change notification settings - Fork 605
implement pretty-printing with {:#}
#1847
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
{:#}
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.
TLDR is I think this is a great feature @lovasoa -- thank you so much. I expect it to evolve over time, but this PR has a great foundation
One thought I had was if you would like help adding support for pretty printing other SQL it might be useful to file some tickets explaining / enumerating the other structures. I suspect other people might like to join in and help you
cc @iffyio
f.write_str("WHEN ")?; | ||
self.condition.fmt(f)?; | ||
f.write_str(" THEN")?; | ||
SpaceOrNewline.fmt(f)?; |
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.
that is pretty cool
I wonder about how you will connect this back to your original goal of preserving the original whitespace.
Perhaps eventually this could be passed the original span 🤔
/// let ast = Parser::parse_sql(&GenericDialect, sql).unwrap(); | ||
/// | ||
/// // Regular formatting | ||
/// assert_eq!(format!("{}", ast[0]), "SELECT a, b FROM table_1"); |
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.
this is awesome
use core::fmt::{self, Display, Write}; | ||
|
||
/// A wrapper around a value that adds an indent to the value when displayed with {:#}. | ||
pub(crate) struct Indent<T>(pub T); |
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.
since it is crate private we can change it later, but I think it would likely be helpful eventually to make the indent configurable (for embedded subqueries, for example)
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.
Unfortunately we cannot really configure much if we are using rust's existing Display trait...
//! | ||
//! # Pretty Printing | ||
//! | ||
//! SQL statements can be pretty-printed with proper indentation and line breaks using the alternate flag (`{:#}`): |
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.
❤️
|
||
#[test] | ||
fn test_pretty_print_select() { | ||
assert_eq!( |
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.
this is great
I'd love if we could merge this one and then I'll follow up with improvements |
I think this is pretty non controversial so let's merge it. @iffyio let us know if you would like any changes |
Great, thank you @alamb ! |
Now,
can display
I am pretty happy about the composability of the resulting code.
Pretty printing is implemented only for select statements, but generalizing this to other statements is almost mechanical and can be done in a subsequent pr.
This will be a real game changer for SQLPage, where error messages are currently often hard to read because of long sql strings.
@alamb @iffyio