-
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
fix comparision for new model/body #4631
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import abc | ||
from itertools import chain | ||
from pathlib import Path | ||
from typing import Set, List, Dict, Iterator, Tuple, Any, Union, Type, Optional | ||
from typing import Set, List, Dict, Iterator, Tuple, Any, Union, Type, Optional, Callable | ||
|
||
from dbt.dataclass_schema import StrEnum | ||
|
||
|
@@ -478,42 +478,28 @@ def check_macros_modified(self, node): | |
previous_macros = [] | ||
return self.recursively_check_macros_modified(node, previous_macros) | ||
|
||
def check_modified(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
# TODO check modifed_content and check_modified macro seems a bit redundent | ||
def check_modified_content(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
different_contents = not new.same_contents(old) # type: ignore | ||
upstream_macro_change = self.check_macros_modified(new) | ||
return different_contents or upstream_macro_change | ||
|
||
def check_modified_body(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
if hasattr(new, "same_body"): | ||
return not new.same_body(old) # type: ignore | ||
else: | ||
return False | ||
|
||
def check_modified_configs(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
if hasattr(new, "same_config"): | ||
return not new.same_config(old) # type: ignore | ||
else: | ||
return False | ||
|
||
def check_modified_persisted_descriptions( | ||
self, old: Optional[SelectorTarget], new: SelectorTarget | ||
) -> bool: | ||
if hasattr(new, "same_persisted_description"): | ||
return not new.same_persisted_description(old) # type: ignore | ||
else: | ||
return False | ||
|
||
def check_modified_relation( | ||
self, old: Optional[SelectorTarget], new: SelectorTarget | ||
) -> bool: | ||
if hasattr(new, "same_database_representation"): | ||
return not new.same_database_representation(old) # type: ignore | ||
else: | ||
return False | ||
|
||
def check_modified_macros(self, _, new: SelectorTarget) -> bool: | ||
return self.check_macros_modified(new) | ||
|
||
@staticmethod | ||
def check_modified_factory( | ||
compare_method: str | ||
) -> Callable[[Optional[SelectorTarget], SelectorTarget], bool]: | ||
# get a function that compares two selector target based on compare method provided | ||
def check_modified_things(old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
if hasattr(new, compare_method): | ||
# when old body does not exist or old and new are not the same | ||
return not old or not getattr(new, compare_method)(old) # type: ignore | ||
else: | ||
return False | ||
return check_modified_things | ||
|
||
def check_new(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | ||
return old is None | ||
|
||
|
@@ -527,14 +513,21 @@ def search( | |
|
||
state_checks = { | ||
# it's new if there is no old version | ||
'new': lambda old, _: old is None, | ||
'new': | ||
lambda old, _: old is None, | ||
# use methods defined above to compare properties of old + new | ||
'modified': self.check_modified, | ||
'modified.body': self.check_modified_body, | ||
'modified.configs': self.check_modified_configs, | ||
'modified.persisted_descriptions': self.check_modified_persisted_descriptions, | ||
'modified.relation': self.check_modified_relation, | ||
'modified.macros': self.check_modified_macros, | ||
'modified': | ||
self.check_modified_content, | ||
'modified.body': | ||
self.check_modified_factory('same_body'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if you think we should go change the underlying function names instead of using the factory here! they are used somewhere but I think it is not too much There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the factory. It avoids duplicating the code in the functions. |
||
'modified.configs': | ||
self.check_modified_factory('same_config'), | ||
'modified.persisted_descriptions': | ||
self.check_modified_factory('same_persisted_description'), | ||
'modified.relation': | ||
self.check_modified_factory('same_database_representation'), | ||
'modified.macros': | ||
self.check_modified_macros, | ||
} | ||
if selector in state_checks: | ||
checker = state_checks[selector] | ||
|
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.
@gshank do you know what's the main difference between these two functions? seems a little bit confusing here
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.
I think the way you're doing this is fine. The
check_modified
method (state:modified
for users) is meant to be the superset of all checks. Renaming this so it clearly maps tosame_contents
(the umbrella check) makes sense