Validate exposure names and add label #5844
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
| @@ -0,0 +1,7 @@ | |||
| kind: Under the Hood | |||
There was a problem hiding this comment.
Not sure where this falls in terms of changelog type?
There was a problem hiding this comment.
This might really be a Breaking Change - but I have an idea of how we could make it not one
There was a problem hiding this comment.
Since we're deprecating instead of erroring now, this won't be breaking. I'll leave as Under The Hood unless you disagree.
There was a problem hiding this comment.
Thanks for this @emmyoop! Sorry I missed your questions the other
As a follow-up to this, let's also start using label instead of name for identifying exposures in dbt-docs, similar to the changes in dbt-labs/dbt-docs#285
core/dbt/contracts/graph/unparsed.py
Outdated
| if errors: | ||
| raise ParsingException( | ||
| f"Exposure name '{data['name']}' is invalid. It {', '.join(e for e in errors)}" | ||
| ) |
There was a problem hiding this comment.
Could we actually make this deprecations.warn instead of ParsingException? I think there are many users (including this integration) who will be in violation of the new rule.
I can provide copy for the deprecation message, but the big idea is: "Hey! We just added support for a human-friendly label propertyon exposures. We see that {exposure} has some disallowed characters in its name — why not turn that into label, and then come up with a simpler name?"
core/dbt/deprecations.py
Outdated
| # todo: word this better - it's bad. Also do we want to set a timeline for when | ||
| # this will become an error? | ||
| _description = """\ | ||
| Exposure names may not contain spaces and can only contain letters, numbers and underscores. | ||
| {exposure} does not follow this pattern. Please use the label attribute for more complex | ||
| names. | ||
| """ |
There was a problem hiding this comment.
@jtcohen6 please provide the promised wording 😄
There was a problem hiding this comment.
How about this:
_description = """\
Starting in v1.3, the 'name' of an exposure should contain only letters, numbers, and underscores.
Exposures support a new property, 'label', which may contain spaces, capital letters, and special characters.
{exposure} does not follow this pattern.
Please update the 'name', and use the 'label' property for a human-friendly title.
This will raise an error in a future version of dbt-core.
"""In the case of metrics, we'd explicitly documented the metric spec as "experimental," and reserved the right to make breaking changes. In that context, it's nice for us to provide backward compatibility for one minor version, and reasonable to expect a quick turnaround (breaking change by v1.4).
Exposures have been around for longer, and without an "experimental" denotation — so I don't think we should commit to a quick turnaround for actual deprecation. The soonest I could see doing this is if we significantly expand the capabilities of exposures, as part of a potential initiative next year. Otherwise, in dbt v2.0.
Given that, I also don't feel the need to open an issue to track this. We'll have an opportunity to revisit this either when we reinvest in exposures, or when we revisit all of our deprecations ahead of v2 (just as we did ahead of v1).
emmyoop
force-pushed
the
er/ct-1184-exposure-name-validation-label
branch
from
3edbcb7
to
2a946fa
Compare
emmyoop
force-pushed
the
er/ct-1184-exposure-name-validation-label
branch
from
2a946fa
to
0f7a83e
Compare
resolves #5606
Description
Adds label and name validation for exposures. Kept as a separate PR to keep the label addition with the name validation.
Open Question
Exactly what do we want to validate as a name? I just copied over from metrics but that's likely incorrect.
Checklist
changie newto create a changelog entry