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

Fix more jinja quoting sadness #2599

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jun 29, 2020

resolves #2597

Description

Per Drew's suggestion, if the input to ast.literal_eval is a str and the output is a str, just return the initial value. I added some tests for the interaction of the native renderer with yaml to make sure they make sense.

As usual, we are kind of caught in a corner with yaml + jinja + ast.literal_eval together, and their slightly different interpretations of rendering. Note the irritating corner cases around differences between true and True and 1: Getting an unquoted string value of 1 still requires as_text, but you can get a true or True out without it (frequently this requires some unintuitive formulations).

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Jun 29, 2020
@beckjake beckjake force-pushed the fix/more-jinja-quoting-sadness branch from bb31204 to d374ef9 Compare June 29, 2020 19:19
@beckjake beckjake marked this pull request as ready for review June 29, 2020 19:47
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment here around an additional test that might be worth adding, but this otherwise LGTM!

('''foo: "{{ true }}"''', True),
('''foo: "{{ 'true' }}"''', 'true'),
('''foo: "'{{ true }}'"''', "'True'"),
('''foo: "{{ true | as_text }}"''', "True"), # true -> boolean True -> text -> str(True) -> 'True'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is surprising, but i guess that's what we should expect if true == True!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put that comment there because my initial reaction to this behavior was that it just had to be wrong 😆

('''foo: "{{ a_str ~ 100 }}"''', 100100),
('''foo: "{{ a_int ~ 100 }}"''', 100100),
('''foo: "{{ a_str }}{{ a_str }}"''', 100100),
('''foo: "{{ a_int }}{{ a_int }}"''', 100100),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add one more test here for:

'''foo: "'{{ a_int }}{{ a_int }}'"'''

I would expect to see "100100" (as a string) but it's worth 1) double checking and 2) adding the test for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually ends up as the string '100100' (including quotes) - it's similar to the '''foo: "'{{ 1 }}'"''' test case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am so sorry, I got the example wrong. You're right - I'd expect to see the single quotes in here too! '100100' is indeed what this should render out to

@beckjake beckjake merged commit a201fc7 into dev/0.17.1 Jun 30, 2020
@beckjake beckjake deleted the fix/more-jinja-quoting-sadness branch June 30, 2020 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants