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

Make dbt support documentation for snapshots and seeds (and analyses) (#1974) #2051

Merged
merged 7 commits into from
Jan 24, 2020

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jan 15, 2020

Fixes #1974

Adds threetwo new sections to schema.yml: snapshots, and seeds, and analyses. They act just like "models", except they must refer to resources of the appropriate type. dbt checks correctness at patch-time.

For backwards compatibility, this PR allows snapshots/seeds /analyses tests+docs to be defined under 'models', but with a deprecation warning. There's no warning for models described by "models", of course!

I did some minor internal refactoring of the schema.yml parser, by creating two custom parser classes to handle sources vs ... "refables" I guess you could call them? I imagine creating a third for macros in the future, with similar behavior but no support for "column tests", etc.

@cla-bot cla-bot bot added the cla:yes label Jan 15, 2020
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great! I did some functional testing here, and I really love the way this feels in practice. Some copy comments here, and a couple of other minor things to note:

If a test references a node that doesn't exist, dbt shows:

WARNING: Test 'test.my_project.unique_name_id' (models/schema.yml) depends on model 'name' which was not found

Can we swap out this text for something like:

WARNING: Test 'test.my_project.unique_name_id' (models/schema.yml) depends on a node named 'name' which was not found.

Minor, but more sensible with some of the changes taking place in this PR.

Also, looks like this code allows you to define columns for an analysis node. This is groovy, but can we disallow the specification of tests on those columns (and on the resource itself)? I can't think of a good reason to support the definition of tests on analyses, just insofar as dbt can't actually run them!

'models-key-mismatch', patch=patch, node=node
)
else:
raise_compiler_error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like

'{node.name}' is a {node.resource_type} node, but it is specified in the
{patch.yaml_key} section of {node.original_file_path}.

To fix this error, place the `{node.name}` specification under the {patch.yaml_key} key instead.

_name = 'models-key-mismatch'

_description = '''
patch instruction in {patch.original_file_path} under key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"{node.name}" is a {node.resource_type} node, but it is specified in the
models section of {node.original_file_path}.

To fix this warning, place the `{node.name}` specification under the {patch.yaml_key} key instead.
This warning will become an error in a future release.

Add a deprecation warning for snapshots/seeds documented under "models"
Add a bunch of tests
@beckjake
Copy link
Contributor Author

Also, looks like this code allows you to define columns for an analysis node. This is groovy, but can we disallow the specification of tests on those columns (and on the resource itself)? I can't think of a good reason to support the definition of tests on analyses, just insofar as dbt can't actually run them!

I think I'm actually going to disable support for analysis nodes for now since that's extra work that nobody probably cares about? I'd need to make a new patch type that includes docs but not tests - I'll have to do similar for macros, so I'll look into it then.

@drewbanin
Copy link
Contributor

Sounds good - makes sense to hit analysis docs at the same time as macro docs.

add support for newlines in output
line-wrap deprecations
update tests
appease mypy
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments about the formatting with linewrap, but this will LGTM when that's addressed!

core/dbt/contracts/graph/manifest.py Show resolved Hide resolved
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, ship it!

@beckjake beckjake merged commit 9d5488e into dev/barbara-gittings Jan 24, 2020
@beckjake beckjake deleted the feature/docs-for-everyone branch January 24, 2020 16:46
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.

2 participants