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

[bug] dbt_utils.group_by gets trailing whitespace removed, breaking queries #165

Closed
rjay98 opened this issue Apr 22, 2022 · 3 comments
Closed
Labels
bug Something isn't working
Milestone

Comments

@rjay98
Copy link

rjay98 commented Apr 22, 2022

Describe the bug
A clear and concise description of what the bug is.

It looks like whitespace is removed from {{ dbt_utils.group_by }}, sticking it directly into the clause above (a where clause, for example), which breaks the query.

To Reproduce
Steps to reproduce the behavior. If this is a formatting issue, include the input code here.

If you sqlfmt a file that looks like this:

select
  ...
from <table_name>
where <where_clause>
{{ dbt_utils.group_by(10) }}

Expected behavior
A clear and concise description of what you expected to happen. If this is a formatting issue, include the expected formatting here, by manually formatting your code from above.

select
  ...
from <table_name>
where <where_clause>
{{ dbt_utils.group_by(10) }}

Actual behavior
Provide any output generated by sqlfmt here. If this is a formatting issue, include the code generated by sqlfmt.

select
  ...
from <table_name>
where <where_clause>{{ dbt_utils.group_by(10) }}

Additional context
What is the output of sqlfmt --version?

0.6.0

@tconbeer tconbeer added the bug Something isn't working label Apr 24, 2022
@tconbeer
Copy link
Owner

Thanks for the report, @rjay98 !

This is pretty similar in root cause to #162. We obviously need to insert a space before a jinja expression when we remove a newline, which would at least make this syntax not break.

Keeping dbt_utils.group_by on its own line will be harder. I've thought about this before, and I don't have a great solution except for hard-coding it in as an exception (it's one of very few macros that start with a keyword). sqlfmt doesn't have any way to know that the macro compiles to a keyword, so it wants to one-line it with the end of the where statement

other than the hacky/hard-coded method, we could consider an inline comment that could tell sqlfmt how to treat a jinja expression (if the default treatment doesn't work). So something like:

{{ dbt_utils.group_by(6) }}  -- fmt: group by

If anyone else has solutions/ideas, I'm open to them, but I do not want to go down the sqlfluff road and jinja-template the sql.

For now, I'd suggest either 1) using a -- fmt: off comment on this line, or b) not using dbt_utils.group_by() and instead implement a macro called range:

{%- macro range(n) -%}
{% for i in range(1, n + 1) -%}
    {{ i }}{{ ',' if not loop.last }}   
{%- endfor -%}
{%- endmacro -%}

and then call it with:

...
where my_condition
group by {{ range(10) }}

@tconbeer
Copy link
Owner

tconbeer commented May 4, 2022

I'm going to separate this into two issues -- the bug where we remove all of the whitespace will stay here; the enhancement to format {{ group_by}} like group by will move into the new issue, #172

@tconbeer
Copy link
Owner

tconbeer commented May 4, 2022

this patch has been released in v0.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants