Skip to content
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

Merged

Conversation

joellabes
Copy link
Contributor

@joellabes joellabes commented Nov 27, 2020

resolves #2913

Description

Should ensure that docs blocks render correctly in exposures' descriptions.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Nov 27, 2020
@joellabes joellabes changed the title Follow Jeremy's wild speculation Render docs blocks in exposures Nov 28, 2020
@joellabes joellabes marked this pull request as ready for review November 28, 2020 02:09
@joellabes
Copy link
Contributor Author

@jtcohen6 I've tested this works with my failing example in #2913. I'm not sure what unit tests I'll need to add or how to go about that, so expecting another couple of rounds of feedback before this can be merged.

Copy link
Contributor

@jtcohen6 jtcohen6 left a 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:

https://github.com/fishtown-analytics/dbt/blob/fec0e31a25d5b922cb1833cffcb5095eb4ee642b/test/unit/test_contracts_graph_unparsed.py#L574-L605

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.

core/dbt/parser/manifest.py Show resolved Hide resolved
@joellabes
Copy link
Contributor Author

This all makes sense, will check it out soon!

That will also require adding the "expected" value of the new exposure here.

If there's already two "normal" exposures documented (simple_exposure and notebook_exposure), why is test_docs_generate's exposures object currently empty? Should I be fleshing it out to contain the other two as well?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Nov 30, 2020

If there's already two "normal" exposures documented (simple_exposure and notebook_exposure), why is test_docs_generate's exposures object currently empty? Should I be fleshing it out to contain the other two as well?

Those "normal" exposures are defined for the "normal" models test, but there aren't any exposures defined for the ref_models test, i.e. the one that tests various resources using doc blocks.

@joellabes
Copy link
Contributor Author

OK I've made those changes, but when I try to run the tests locally it doesn't work:

(env) Serenity:dbt joel$ make test-unit
Unit test run starting...
docker-compose: No such file or directory
        0.00 real         0.00 user         0.00 sys
make: *** [test-unit] Error 127

The contributing guide assumes more prior knowledge than I have for making this go unfortunately

Copy link
Contributor

@jtcohen6 jtcohen6 left a 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.

core/dbt/contracts/graph/unparsed.py Outdated Show resolved Hide resolved
@@ -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',
Copy link
Contributor Author

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

Copy link
Contributor

@jtcohen6 jtcohen6 left a 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:

https://github.com/fishtown-analytics/dbt/blob/34869fc2a2a354a18a232e21315c3901aafab0b6/test/integration/029_docs_generate_tests/test_docs_generate.py#L2086-L2101

You can check the exposures in the previous test to see how they should appear in parent/child map.

Very close!

@joellabes
Copy link
Contributor Author

Copy link
Contributor

@jtcohen6 jtcohen6 left a 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

@jtcohen6 jtcohen6 merged commit cd149b6 into dbt-labs:dev/kiyoshi-kuromiya Dec 13, 2020
@joellabes
Copy link
Contributor Author

Couldn't have done it without you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs blocks don't work in exposures - 'doc' is undefined
2 participants