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

sql-pretty: add pretty printing crate #22251

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

maddyblue
Copy link
Contributor

This is copied from github.com/mjibson/mzfmt but now integrated directly into our repo. Refactor the parser tests to reuse the datadriven statements for verification here. That will prevent future syntax changes from lagging behind in their pretty implementation.

Integration into SHOW and other commands in future commits.

Motivation

  • This PR adds a feature that has not yet been specified.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • n/a

@maddyblue maddyblue requested a review from a team as a code owner October 8, 2023 07:41
@maddyblue maddyblue requested review from a team and ParkMyCar October 8, 2023 07:41
@maddyblue maddyblue force-pushed the pretty branch 3 times, most recently from 3c18303 to 373a694 Compare October 9, 2023 06:42
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet!

.group()
}

//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraneous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an intended comment to separate util functions from doc functions. I've made those separate files instead.

@maddyblue maddyblue force-pushed the pretty branch 4 times, most recently from 37a33fe to 577688b Compare October 10, 2023 17:09
This is copied from github.com/mjibson/mzfmt but now integrated directly
into our repo. Refactor the parser tests to reuse the datadriven
statements for verification here. That will prevent future syntax
changes from lagging behind in their pretty implementation.

Integration into `SHOW` and other commands in future commits.
@maddyblue
Copy link
Contributor Author

I've also rejiggered the cargo.toml dependencies so parser didn't get any new dependencies in its production use path, but now has a test feature that is only used by test builds.

@maddyblue maddyblue merged commit 22a8740 into MaterializeInc:main Oct 10, 2023
63 checks passed
@maddyblue maddyblue deleted the pretty branch October 10, 2023 21:07
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.

2 participants