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
Update DbtTemplater to use JinjaTracer #1788
Update DbtTemplater to use JinjaTracer #1788
Conversation
… at once by generate()
…r' of https://github.com/barrywhart/sqlfluff into bhart-issue_1783_update_dbt_templater_to_use_jinjatracer
I'm not hitting the second error, either. I'm providing some info for context -- maybe we are running different versions of dbt or dbt_utils ... ? SQLFluff repo:
dbt and SQLFluff package versions:
Here's a stack trace and the
|
I wonder if my issues are actually due to the way I've installed dbt from git. I'll see if I can replicate them in a cleaner environment, |
Thanks! Related: Would you like me to add your two examples as automated test cases? |
Yes please actually - if they're in the test dbt package and work fine there - then I'm fine for this to be merged without more testing. That's all I was going to do anyway :) |
…r' of https://github.com/barrywhart/sqlfluff into bhart-issue_1783_update_dbt_templater_to_use_jinjatracer
@alanmcruickshank: I added both tests. The first was no problem. The second one (the one using This was the error:
I modified your example to use a different dbt_utils macro, |
@barrywhart you definitely don't want this in 0.8.0? @WittierDinosaur is preparing that release now. |
@tunetheweb: I'm okay either way. @alanmcruickshank tested this on one of his dbt projects. I'd say don't wait on this, but if Alan approves it in time, let's include it. |
…r' of https://github.com/barrywhart/sqlfluff into bhart-issue_1783_update_dbt_templater_to_use_jinjatracer
@@ -12,4 +12,4 @@ where not products._fivetran_deleted | |||
{% if true -%} | |||
and products.valid_date_local >= ( | |||
select max(valid_date_local) from {{ this }}) | |||
{% endif -%} | |||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This started breaking after the recent L009 PR was merged. IIUC, using whitespace control at the end of the file caused all the trailing newlines to be (correctly) removed by Jinja, thus L009 would report an issue. I feel comfortable with this change, i.e. that the behavior was correct and that this fix makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks for the additional test cases. I think this is good to go!
Brief summary of the change made
Fixes #1783
This PR builds on #1678. That one adds
JinjaTracer
and updates the Jinja templater to use it, and this one updates the dbt templater to use it. We may want to consider delaying merge of this PR until we've had a release with that one. More people use dbt, and dbt users are often less technical, so this strategy lets us do a sort of "phased" release.Are there any other side effects of this change that we should be aware of?
...
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.yml
rule test cases intest/fixtures/rules/std_rule_cases
..sql
/.yml
parser test cases intest/fixtures/dialects
(note YML files can be auto generated withpython test/generate_parse_fixture_yml.py
or by runningtox
locally).test/fixtures/linter/autofix
.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.