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

add a flag to wrap models/tests in jinja blocks #1407

Merged
merged 5 commits into from
Apr 18, 2019

Conversation

beckjake
Copy link
Contributor

And then run all tests through that. Everything seems to have worked fine!

Fixes #1400

Jacob Beck added 2 commits April 16, 2019 09:45
test for comment blocks where the first character of the comment is "}"
do/set tests
an even trickier test case
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
'Found invalid block contents when parsing node {} ({}) '
'from its wrapped block, expected it to match original file'
.format(name, path)
)
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@beckjake beckjake force-pushed the feature/wrap-models-in-jinja-tags branch from c90ae55 to 4ffc5cb Compare April 18, 2019 17:48
@beckjake beckjake merged commit dd469ad into dev/wilt-chamberlain Apr 18, 2019
@beckjake beckjake deleted the feature/wrap-models-in-jinja-tags branch April 18, 2019 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap DBT models/tests in `{% model %}...{% endmodel %} blocks
2 participants