-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
add a flag to wrap models/tests in jinja blocks #1407
add a flag to wrap models/tests in jinja blocks #1407
Conversation
After we find the start of a comment block, advance immediately - this is so we do not mistake "{#}" as both open and close of comment support do/set statements (no {% enddo %}/{% endset %}) fix some edge-case bugs around quoting fix a bug around materialization parsing
core/dbt/parser/base.py
Outdated
'Found invalid block contents when parsing node {} ({}) ' | ||
'from its wrapped block, expected it to match original file' | ||
.format(name, path) | ||
) |
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 is interesting, but it seems almost tautological.... unless someone included an unclosed comment or a {% endmodel %}
in their model i would expect this to always pass. I am more interested in just running the toplevel block parser over the unwrapped model contents, and comparing any compiler errors to those raised by the jinja parser. if the block parser raises an error, but the jinja parser does not, then we have a bug that we need to fix.
you should do the exact same thing for macro files too. --wrap-models-in-tags
is probably a bad name if this turns the parser on for macros and models, so maybe --test-new-parser
or something like that would be a better name
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.
unless someone included an unclosed comment or a
{% endmodel %}
in their model i would expect this to always pass
Well, me too, but I thought the idea was to shake out bugs around top-level block extraction that are in that category. I guess I misunderstood - I'll change it to be the way you've described, which seems like a reasonable enough test.
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.
@beckjake gotcha. I care more about comparing our parser to jinja's parser on arbitrary jinja code, not necessarily testing model block extraction. unrelated: i think wrapping model files in model blocks is a good way to achieve forwards compatibility with this new parser.
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.
Yup! I that is the longer-term plan, I think. Maybe we can get it in for wilt-chamberlain if there isn't too much already, that would be nice.
c90ae55
to
4ffc5cb
Compare
And then run all tests through that. Everything seems to have worked fine!
Fixes #1400