Add documentation for macros/analyses (#1041) #2068
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beckjake
commented
Jan 24, 2020
core/dbt/contracts/graph/manifest.py
Outdated
| patch = patches.pop(node.name, None) | ||
| if not patch: | ||
| continue | ||
| expected_key = node.resource_type.pluralize() | ||
| if expected_key == patch.yaml_key: | ||
| node.patch(patch) |
There was a problem hiding this comment.
we used to double-patch() - github hides the other call by default
beckjake
changed the base branch from
feature/docs-for-everyone
to
dev/barbara-gittings
beckjake
force-pushed
the
feature/macro-docs
branch
from
8660f17
to
abab3c7
Compare
|
I just rebased this against dev/barbara-gittings, resolved a few merge conflicts along the way. |
beckjake
force-pushed
the
feature/macro-docs
branch
from
abab3c7
to
8332674
Compare
drewbanin
reviewed
Jan 30, 2020
There was a problem hiding this comment.
This looks great! Some comments inline on this one, but all really minor.
Some bigger picture thoughts as I looked through this one:
- It would be killer if dbt supported the specification of macro arguments + types in a
macros:block. This would be really great for things like dbt-utils. We could use this info to render docs + really good editor integrations in the future. I'm going to make a separate issue to track that idea. (Describe macro arguments in schema.yml files #2081) - In testing out this PR, I noticed that macros are not rendering in the
depends_onlist for models in the rendered manifest. I don't know if this is a recent regression (it definitely was not introduced in this branch), but it's worth fixing so that we can render the referenced macros for a model in the docs site. I'll create a new issue for that one too. (Add macros to depends_on list for nodes that can call macros #2082) - Another one from testing: If a
description:is provided which contains adoc()call, then the docs are rendered with jinja. If the description instead contains an arbitrary jinja expression (eg.{{ 1 + 1}}) then dbt renders that literal string{{ 1 + 1 }}into the description field for the node. That's pretty weird, right? I will.... create an issue :) (Weird rendering behavior for schema.yml descriptions #2083)
core/dbt/contracts/graph/manifest.py
Outdated
| # target_not_found warning | ||
| logger.debug(( | ||
| 'WARNING: Found documentation for macro "{}" which was ' | ||
| 'not found or is disabled').format(patch.name) |
There was a problem hiding this comment.
Suggested change
| 'not found or is disabled').format(patch.name) | |
| 'not found').format(patch.name) |
core/dbt/contracts/graph/manifest.py
Outdated
| for patch in patches.values(): | ||
| # since patches aren't nodes, we can't use the existing | ||
| # target_not_found warning | ||
| logger.debug(( |
There was a problem hiding this comment.
Can we elevate this to a proper warning?
core/dbt/exceptions.py
Outdated
| raise_compiler_error( | ||
| f'dbt found two schema.yml entries for the same macro in package ' | ||
| f'{package_name} named {name}. Macros may only be described a single ' | ||
| f'time. To fix this, remove the macros entry for for {name} in one ' |
There was a problem hiding this comment.
Suggested change
| f'time. To fix this, remove the macros entry for for {name} in one ' | |
| f'time. To fix this, remove the macros entry for {name} in one ' |
This was referenced Jan 31, 2020
Refactored parsing a bit to support the idea of: - macro patches (no columns) - node patches that don't get tests (analyses)
beckjake
force-pushed
the
feature/macro-docs
branch
from
8332674
to
704b345
Compare
drewbanin
approved these changes
Jan 31, 2020
beckjake
force-pushed
the
feature/macro-docs
branch
from
704b345
to
04ebdbb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1041
Add and populate a
documentationfield to macrosPopulate the
documentationfield on analyses like we do with models/seeds/snapshotsRefactored parsing a bit to support the idea of: