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

add a "docs" field to models (#1671) #2107

Merged
merged 1 commit into from
Feb 11, 2020

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Feb 7, 2020

Fixes the dbt-core side of #1671

It's parsed from the "models" block in schema.yml. It contains one field, a bool named "show" which defaults to True.

version: 2

models:
  - name: model
    description: "The test model"
    docs:
      show: false

This has no impact besides adding the field to compiled output, and this PR does not update the docs page to make any use of the field.

@cla-bot cla-bot bot added the cla:yes label Feb 7, 2020
@beckjake beckjake force-pushed the feature/node-docs-show branch 2 times, most recently from ae3ef19 to 997e698 Compare February 7, 2020 23:34
@mike-weinberg
Copy link

@beckjake @drewbanin just curious if you saw #1671 (comment) -- considerations for forward compatibility with potential future doc configuration features

@beckjake
Copy link
Contributor Author

beckjake commented Feb 10, 2020

@mike-weinberg I didn't notice it, but show vs hide is more of an aesthetic thing, right? And with this model it seems pretty straightforward to do the same thing as you suggested, except inverted (opt-in instead of out once you commit to making changes):

docs:
   show: lineage

or

docs:
    show:
        - lineage

or whatever else we want to support. So I guess I feel pretty good about forwards compatibility, unless there's something I'm missing?

But of course I will defer to drew on this - I don't have any strong feelings on the matter, and it's easy enough to change.

Edit: Obviously, we'd have to change the code to accept a bool or an enum, but this is a "we could support with this syntax", not a "we will support".

@beckjake beckjake removed the request for review from drewbanin February 10, 2020 15:47
@beckjake
Copy link
Contributor Author

I'll rebase this - I kind of expected removing docrefs to conflict with this new field.

…oolean "show" which defaults to True.

Clean up the contracts tests a bit
@drewbanin
Copy link
Contributor

@mike-weinberg let me hit you back in the open issue

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 LGTM - ship it!

@beckjake beckjake merged commit 89c65af into dev/barbara-gittings Feb 11, 2020
@beckjake beckjake deleted the feature/node-docs-show branch February 11, 2020 18:04
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.

3 participants