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

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

Closed
ramonvermeulen opened this issue Jun 26, 2023 · 5 comments · Fixed by #452
Closed

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

ramonvermeulen opened this issue Jun 26, 2023 · 5 comments · Fixed by #452
Labels
bug Something isn't working

Comments

@ramonvermeulen
Copy link

ramonvermeulen commented Jun 26, 2023

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
Copy link
Owner

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

@tconbeer tconbeer added the bug Something isn't working label Jun 26, 2023
@ramonvermeulen
Copy link
Author

ramonvermeulen commented Jun 27, 2023

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
Copy link
Owner

thank you! This is extremely helpful

tconbeer added a commit that referenced this issue Jul 13, 2023
* chore: bump deps

* fix #447: early stop all fmt options if fmt is disabled

* fix #136: don't format comments in fmt off blocks

* fix: clean up newline handling in comments'

* fix: restore comment formatting for data tokens
@tconbeer
Copy link
Owner

@ramonvermeulen This has been patched in v0.19.1

@ramonvermeulen
Copy link
Author

@ramonvermeulen This has been patched in v0.19.1

Awesome, thanks for picking this up so fast!

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

Successfully merging a pull request may close this issue.

2 participants