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

{{ exceptions.warn() }} returns None instead of an empty string #2222

Closed
clrcrl opened this issue Mar 19, 2020 · 4 comments · Fixed by #2259
Closed

{{ exceptions.warn() }} returns None instead of an empty string #2222

clrcrl opened this issue Mar 19, 2020 · 4 comments · Fixed by #2259
Labels
bug Something isn't working good_first_issue Straightforward + self-contained changes, good for new contributors!

Comments

@clrcrl
Copy link
Contributor

clrcrl commented Mar 19, 2020

Describe the bug

{{ exceptions.warn() }} returns None instead of an empty string (which is what I expected)

Steps To Reproduce

1:

$ cat models/my_model.sql
{{ exceptions.warn("bad thing") }}
select 1
  1. Compile the SQL, dbt compile

Expected behavior

I expected to get a warning in my terminal, but the SQL should get compiled like so:

select 1

Actual behavior

A None gets inserted, resulting in invalid SQL

None
select 1

The output of dbt --version:
0.16.0-rc2

The operating system you're using:

The output of python --version:

Additional context

Can workaround by using the {% do %} syntax (as documented here, which I just updated).

So I'm okay with this getting a #wontfix on it.

@clrcrl clrcrl added bug Something isn't working triage labels Mar 19, 2020
@drewbanin drewbanin added good_first_issue Straightforward + self-contained changes, good for new contributors! and removed triage labels Mar 19, 2020
@drewbanin drewbanin added this to the Octavius Catto milestone Mar 19, 2020
@drewbanin
Copy link
Contributor

Good idea @clrcrl - let's do this one for the Octavius Catto release :)

@jeremyyeo
Copy link
Contributor

Hey all looking for some pointers on trying to make my first contribution (also newish to Python) and seems like this would be an okay one to attempt (maybe?).

Managed to find that if I added a compiled_node.compiled_sql = re.sub("None", "", compiled_node.compiled_sql) in compilation.py then it managed to compile the SQL correctly but rendering the macro not functional. This is most certainly not the right way to do this.

Traced it further back to the get_template / render_template in jinja.py but can't quite understand what's going on here... is it because Jinja parses these 2 differently?

{% do exceptions.warn("warning") %} 
{{ exceptions.warn("warning") }} 

Keen to know where I should be looking to make the changes. Thanks :)

@beckjake
Copy link
Contributor

Hi @jeremyyeo, great to hear you're working on this! I would change the function definition of warn in core/dbt/exceptions.py from return run_or_error(msg, node=node) to:

run_or_error(msg, node=node)
return ''

I think that's going to be a lot easier than trying to remove Nones from the compiled sql!

@beckjake
Copy link
Contributor

Oh, and I neglected your second part of the question! Yes, there is a difference! {% do ... %} always evaluates to the empty string in jinja. On the other hand, {{ ... }} evaluates to str(...). So the trick is to make exceptions.warn return something that will evaluate to the empty string. We do something similar in log (see core/dbt/contexts/base.py).

beckjake added a commit that referenced this issue Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good_first_issue Straightforward + self-contained changes, good for new contributors!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants