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 dbt documentation to sale.* assets #202

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Conversation

dfsnow
Copy link
Member

@dfsnow dfsnow commented Oct 31, 2023

This PR adds column documentation to all tables and views in the sale schema. It also adds top-level documentation to each table/view. Finally, it correctly capitalizes MyDec elsewhere in our dbt documentation.

@wagnerlmichael please review this for accuracy.

@dfsnow dfsnow requested a review from a team as a code owner October 31, 2023 00:40
@@ -14,7 +14,7 @@ models:
- name: is_multisale
description: '{{ doc("shared_column_sale_is_multisale") }}'
- name: is_mydec_date
description: Indicator for whether or not the observation uses the myDec sale date
description: Indicator for whether or not the observation uses the MyDec sale date
Copy link
Member Author

Choose a reason for hiding this comment

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

Just capitalizing MyDec correctly in all places in the documentation. Didn't want to put it in a separate PR.

{% enddocs %}

# group_mean
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added markdown headers to the doc assets just to make them easier to read, navigate, and fold.

Copy link
Member

@wagnerlmichael wagnerlmichael left a comment

Choose a reason for hiding this comment

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

Looks good! Added some nitpicks and small suggestions.

Copy link
Contributor

@jeancochrane jeancochrane 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, thanks for this!

dbt/models/sale/docs.md Show resolved Hide resolved
@@ -1,19 +1,67 @@
# flag

{% docs flag %}
Copy link
Contributor

Choose a reason for hiding this comment

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

[Thought, non-blocking] I like the way you've been using prefixes to hint at the purpose of column descriptions, i.e. column_* and shared_column_*. I wonder if that's worth doing for tables, too? E.g. table_flag instead of flag? Views already follow a built-in vw_* naming convention, so it seems like tables are the odd ones out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already started to do that in the upcoming #203! So we're definitely on the same page.

Comment on lines +33 to +36
- name: foreclosure
description: '{{ doc("foreclosure") }}'
tags:
- load_manual
Copy link
Contributor

Choose a reason for hiding this comment

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

[Question, non-blocking] Do we want docs for sale.foreclosure columns as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but there are a billion columns and I don't want to add docs for them right now. I added this as a subtask to #201.

@dfsnow dfsnow merged commit 6940e16 into master Oct 31, 2023
7 checks passed
@dfsnow dfsnow deleted the dansnow/add-sale-db-col-defs branch October 31, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants