Back/fwd compatibility for renamed metrics attributes
#5825
Conversation
|
@emmyoop I added a metric to the project used in |
|
Oh, so we just didn't have metrics defined in the previous project we were using to generate the manifest? Makes sense. |
| AdditionalPropertiesMixin, | ||
| Mergeable, | ||
| Replaceable, | ||
| rename_metric_attr, |
There was a problem hiding this comment.
Just for my own understanding, is there any reason (apart from data class vs function) for the different naming conventions here?
There was a problem hiding this comment.
apart from data class vs function
I think that's it!
|
Thanks for the functional review @callum-mcdata! @emmyoop I'll wait to merge till you have a chance to review the code |
core/dbt/contracts/util.py
Outdated
| if "window" not in metric_content: | ||
| metric_content["window"] = None |
There was a problem hiding this comment.
Instead of this you should change the ParsedMetric definition to default to None for window, same as it already does for UnparsedMetric. I would expect that to resolve this.
While looking at that bit of code, I noticed ParsedMetric still has timestamp as optional and I know @callum-mcdata updated the UnparsedMetric definition to not be optional in a recent PR. timestamp should probably match.
There was a problem hiding this comment.
@emmyoop you were totally right! I was able to remove this component by setting the default value of window to be None in ParsedMetric. I also fixed the timestamp field to reflect UnparsedMetric like you suggested - must have missed that on my last PR!
core/dbt/deprecations.py
Outdated
| _description = """\ | ||
| dbt-core v1.3 renamed attributes for metrics: | ||
| \n 'sql' -> 'expression' | ||
| \n 'type' -> 'calculation_method' | ||
| \n 'type: expression' -> 'calculation_method: derived' | ||
| \nThe old metric parameter names will be fully deprecated in v1.4. | ||
| \nPlease remove them from the metric definition of metric '{metric_name}' | ||
| """ |
There was a problem hiding this comment.
Can we add a comment here with a link to the issue that will do this so it doesn't get lost? Love that we're committing to a timeframe here!
There was a problem hiding this comment.
Issue created here: #5849
There was a problem hiding this comment.
@emmyoop the link to this GH issue has been added to the deprecation warning!
There was a problem hiding this comment.
@callum-mcdata I actually meant a comment in the code, not as part of the deprecation message. Sorry if that was unclear! Unless you think users will find the link to the issue useful?
There was a problem hiding this comment.
🤦 woops! Although now that I think about it, if we include the link to the issue then anyone who feels very strongly that this behavior should be retained could go to it easily and let us know there. That way the information isn't spread over multiple channels..... so actually I do think it would be useful!
|
Thanks for the review @emmyoop, and for getting this over the finish line @callum-mcdata! |
* Naive handling for metric attr renames * Add tests for bwd/fwd compatibility * Add deprecation * Add changelog entry * PR feedback * Small fixups * emmyoop's suggestions Co-authored-by: Callum McCann <[email protected]>
resolves #5807
follow-up to #5775
Description
Define a method,
rename_metric_attr, that switches the old metric attribute names + values (sql,type,type: expression) to the new names + values (expression,calculation_method,calculation_method: derived). This is called in two places:--state), just handle the renameI also removed some annotations within the metrics parsing code that I believe to be outdated
Pedantic question, not very important: We tend to call these things "properties" within dbt, but am I right in thinking the Pythonic name for them is dataclass attributes?
Checklist
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PRchangie newto create a changelog entry