Fix/more helpful invalid refs (#1080) #1085
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drewbanin
reviewed
Oct 23, 2018
dbt/clients/jinja.py
Outdated
| ) | ||
|
|
||
| def __getattr__(self, name): | ||
| if name == 'name' or name.startswith('__') and name.endswith('__'): |
There was a problem hiding this comment.
can you put some parens in here?
There was a problem hiding this comment.
did one better and just made it a function
|
|
||
| def __getattr__(self, name): | ||
| if name == 'name' or name.startswith('__') and name.endswith('__'): | ||
| raise AttributeError( |
There was a problem hiding this comment.
why would we raise this error for __*__?
There was a problem hiding this comment.
because copy.deepcopy does a series of successive hasattr(x, '__*__') checks to decide how to deepcopy things, and hasattr(obj, name) is basically implemented as:
try:
getattr(obj, name)
except AttributeError:
return False
else:
return True
Since __getattr__ is only accessed when an attribute is not already defined, we want to signal that we don't implement those methods.
drewbanin
approved these changes
Oct 23, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1080 - in particular the issue described by @raybuhr
There are some neat things going on here:
ParserMacroCapture.__getattr__accessesself.namewithout ensuring that it is not doing a lookup forself.name-> recursion errorParserMacroCaptureis deepcopied. deepcopy sort of calls new(), and populates the created object, bypassing init through a convoluted set ofhasattrchecks that we pass due to the__getattr__implementation.ref'dThe ways you might think would help (raising AttributeError in getattr, etc) all end up being pretty unhelpful since jinja does propagation of Undefined values, so you get odd TypeErrors down the line. But implementing a
__deepcopy__that raises a compiler error works: jinja won't catch it and the error can be pretty precise/helpful. Any time you find yourself calling copy.deepcopy() with an undefined value, things have gone horribly wrong, so I think it's a valid solution.