tconbeer / sqlfmt

sqlfmt formats your dbt SQL files so you don't have to
https://sqlfmt.com
Apache License 2.0
391 stars 16 forks source link

adding `-- fmt: off` at top of file causes error #447

Closed ramonvermeulen closed 1 year ago

ramonvermeulen commented 1 year ago

Describe the bug In the documentation is stated that "a single -- fmt: off at the top of a file will keep the entire file intact". However it will give an error while running sqlfmt, or sqlfmt --diff in a pipeline. I don't know if this is desired functionality, but I don't think it should be.

Error message: sqlfmt encountered an error: All lines in the segment are empty

If this is not desired functionality, I'm willing to make a pull request to change this.

To Reproduce Put a -- fmt: off comment on the first line of a file, run sqlfmt against that file.

Expected behavior If there is only a -- fmt: off comment on top of the file, and no other -- fmt: on comment in the file, I would like sqlfmt to ignore the full file, and not throw an error with the message sqlfmt encountered an error: All lines in the segment are empty

Actual behavior sqlfmt throws an error: sqlfmt encountered an error: All lines in the segment are empty

Additional context sqlfmt, version 0.19.0

tconbeer commented 1 year ago

this is dependent on the contents of your file. Can you provide the file that is throwing the exception, or a simplified repro?

ramonvermeulen commented 1 year ago

Thanks for your fast reply @tconbeer !

Sure, one of the models where I just replaced names:

-- fmt: off
with abc as (
    select
        *
        {% if env_var('X') == 'Y' %}

                ,dense_rank() over(partition by z order by y desc) as foo
        {% endif %}

    from {{ ref('stg_something')}}

),

abc as (
    select * from
    country_trial
    {% if env_var('X') == 'Y' %}

        where foo = 1
    {% endif %}

)

select * from dim_something

Actually I found out it is working if I remove lines 6 and 18. But still, even with this whitespace it is valid jinja as well as compiled SQL and should not throw an error in my opinion.

I need this functionality because I want new dbt models to enforce best-practices (e.g. snake casing, correctly formatting sql, etc.), but some old legacy models still use camel casing. I can't refactor these in one go because a lot of dashboards (and teams within a company) are depended on them (it is a pretty big dbt project, with over 500 models).

tconbeer commented 1 year ago

thank you! This is extremely helpful

tconbeer commented 1 year ago

@ramonvermeulen This has been patched in v0.19.1

ramonvermeulen commented 1 year ago

@ramonvermeulen This has been patched in v0.19.1

Awesome, thanks for picking this up so fast!