feat(fmt): support SQL #26750
feat(fmt): support SQL #26750
Conversation
|
|
| lines_between_queries: 2, | ||
| uppercase: Some(true), |
There was a problem hiding this comment.
Would it be beneficial to control this options?
For me it would be a nice to have to control the uppercase, as i like my sql to be lowercase, but all good if not.
There was a problem hiding this comment.
I think for the first pass we shouldn't have it; we also have no "knobs" for controlling options for HTML/CSS/YAML formatters for now.
M4RC3L05
force-pushed
the
feature/fmt-sql
branch
from
8370bdf to
0c1860a
Compare
| @@ -0,0 +1,18 @@ | |||
| select * from foo; | |||
There was a problem hiding this comment.
Could you actually move these files to tests/specs/fmt/sql instead? I didn't even know we had a test here and I think we should remove these altogether in favor of using spec tests.
There was a problem hiding this comment.
Is the idea to remove the tests in tests/integration/fmt_tests.rs?
There was a problem hiding this comment.
Yeah, I would remove them altogether, but we can do that in a follow up PR
There was a problem hiding this comment.
other question, how does one run the spec tests? i tried cargo test spec::fmt::sql but did not work i think...
There was a problem hiding this comment.
Just noticed the README lol
There was a problem hiding this comment.
cargo test specs::fmt::sql will work - notice the "s" in specs :)
M4RC3L05
force-pushed
the
feature/fmt-sql
branch
2 times, most recently
from
0b38765 to
ae74f31
Compare
cli/tools/fmt.rs
Outdated
| .to_owned(); | ||
|
|
||
| // Add single new line to the end of file. | ||
| formatted_str.push_str("\n"); | ||
|
|
||
| Ok(Some(formatted_str.to_owned())) |
There was a problem hiding this comment.
It seems we're allocating twice here. Do we really need to trim_end() and then push a new line?
There was a problem hiding this comment.
It appers that sqlformat does not add a new line at the end and there is no option to control that.
In my opinion, it would preferably be better to have this supported by sqlformat directly, i did it here because it does not support.
Gonna open an issue with sqlformat to see if this is something they would like to have support for.
There was a problem hiding this comment.
I guess it's fine then. Maybe just remove the first to_owned() since sqlformat already returns an owned String.
M4RC3L05
force-pushed
the
feature/fmt-sql
branch
from
ae74f31 to
47587b4
Compare
Is there any code testes that need to be done here? |
M4RC3L05
force-pushed
the
feature/fmt-sql
branch
from
47587b4 to
209eaf7
Compare
No, just updating the file is fine 👍 |
Signed-off-by: m4rc3l05 <[email protected]>
M4RC3L05
force-pushed
the
feature/fmt-sql
branch
from
209eaf7 to
fb5e929
Compare
|
Discussed this internally during the CLI meeting; some team members are skeptical because of so-so abilities of the formatter - eg. @lucacasonato raised that this formatter doesn't handle Not clear if we're gonna land it for Deno v2.1; but we'll open some issues with their repo to see if it can be improved. |
|
I spoke with @lucacasonato about it and we tried formatting all SQL queries used in jsr repo: bartlomieju/try-deno-fmt-sql#1. I filed a couple issues with |
There was a problem hiding this comment.
LGTM, thanks @M4RC3L05. Gonna land it as unstable for Deno v2.1.
This PR closes adds support for sql in deno fmt subcommand.
This PR was based on previous PRs adding formatters to other languages.
The idea of this PR is to see if it would be a good idea to support formatting sql files in deno.
Closes: #25024
First time working on any substantial rust code so bear with me :)