-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Render docs blocks in exposures #2920
Render docs blocks in exposures #2920
Conversation
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.
Nice!
I left a comment around a failing mypy test.
The failing unit tests have to do with TestUnparsedExposure
:
In particular, now that the UnparsedExposure
inherits from HasYamlMetadata
, we need to add three more attributes to get_ok_dict
(as JSON) and test_ok
(as arguments to self.ContractType
):
yaml_key: 'exposures',
original_file_path: '/some/fake/path',
package_name: 'test',
That gets us up to status quo ante. As far as a new test, to ensure that this continues to work, I'd recommend that you add a doc
-using exposure here, to parallel what we already have here. That will also require adding the "expected" value of the new exposure here.
This all makes sense, will check it out soon!
If there's already two "normal" exposures documented ( |
Those "normal" exposures are defined for the "normal" |
OK I've made those changes, but when I try to run the tests locally it doesn't work:
The contributing guide assumes more prior knowledge than I have for making this go unfortunately |
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.
No worries! I think you're really close.
As far as local tests: Do you have docker downloaded on your machine?
If you can't get those to work, running tests by pushing commits to CI is also just fine.
…bes/dbt into 2913-docs-block-exposures
@@ -1567,7 +1567,7 @@ def expected_seeded_manifest(self, model_database=None, quote_model=False): | |||
'macros': [], | |||
'nodes': ['model.test.model', 'model.test.second_model'] | |||
}, | |||
'description': 'A description of the complex exposure', | |||
'description': 'A description of the complex exposure\n', |
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 feel like I saw a way to add an ignore-whitespace instruction somewhere, but am doing this for now
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 this latest failure comes from the fact that exposure.test.notebook_exposure
is missing from parent_map
and child_map
for the ref_models
test:
You can check the exposures in the previous test to see how they should appear in parent/child map.
Very close!
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.
Nice work!! Thanks for the contribution @joellabes
Couldn't have done it without you! |
resolves #2913
Description
Should ensure that docs blocks render correctly in exposures' descriptions.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.