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.

Sign up for GitHub

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

Jump to bottom

remove top level meta attribute from SemanticModels #8766

Merged
merged 1 commit into from
Oct 4, 2023
Merged

remove top level meta attribute from SemanticModels #8766

merged 1 commit into from
Oct 4, 2023

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Oct 4, 2023

related to #8511

Problem

meta is a top level attribute in #8754 but doesn't need to be

Solution

Modify to only allow meta under the config key.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

@emmyoop emmyoop requested review from a team as code owners October 4, 2023 13:32
@emmyoop emmyoop requested review from jzhu13 and ChenyuLInx and removed request for a team October 4, 2023 13:32
@cla-bot cla-bot bot added the cla:yes label Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (eac13e3) 86.50% compared to head (1fff2ee) 85.34%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8766      +/-   ##
==========================================
- Coverage   86.50%   85.34%   -1.17%     
==========================================
  Files         176      176              
  Lines       25825    25821       -4     
==========================================
- Hits        22341    22038     -303     
- Misses       3484     3783     +299
Flag Coverage Δ
integration 81.99% <ø> (-1.29%) ⬇️
unit 65.12% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
core/dbt/contracts/graph/nodes.py 93.52% <ø> (-1.58%) ⬇️
core/dbt/contracts/graph/unparsed.py 93.51% <ø> (-0.02%) ⬇️
core/dbt/parser/schema_yaml_readers.py 91.91% <ø> (-0.07%) ⬇️

... and 35 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emmyoop emmyoop mentioned this pull request Oct 4, 2023
Closed
@emmyoop emmyoop requested a review from QMalcolm October 4, 2023 13:36
@emmyoop
Copy link
Member Author

emmyoop commented Oct 4, 2023

Code coverage went down but all I did was remove code.

QMalcolm
QMalcolm approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@QMalcolm QMalcolm 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! Sorry about the thrash 🙃

@emmyoop
Copy link
Member Author

emmyoop commented Oct 4, 2023

@QMalcolm it was a good question and the code is a tiny bit cleaner now! Worth it.

@emmyoop emmyoop merged commit c6ff3ab into main Oct 4, 2023
54 of 55 checks passed
@emmyoop emmyoop deleted the er/remove-legacy-meta-sm branch October 4, 2023 18:23
@dbeatty10
Copy link
Contributor

The current docs for the meta config says the following:

Depending on the resource you're configuring, meta may be available within the config property, or as a top-level key. (For backwards compatibility, meta is always supported as a top-level key, though without the capabilities of config inheritance.)

Since this is no longer accurate, I'll update it here to something like the following:

Depending on the resource you're configuring, meta may be available within the config property, and/or as a top-level key. (For backwards compatibility, meta is often (but not always) supported as a top-level key, though without the capabilities of config inheritance.)

@dbeatty10 dbeatty10 mentioned this pull request Oct 7, 2023
Merged
3 tasks
QMalcolm pushed a commit that referenced this pull request Oct 9, 2023
@emmyoop @QMalcolm
remove top level meta attribute (#8766)
e9481e3
@tlento tlento mentioned this pull request Nov 6, 2023
Merged
5 tasks
matthewshaver added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Nov 8, 2023
@matthewshaver
meta config for semantic models (#4218)
cfca7c4 
related to: #4180
related to: #4281

## What are you changing in this pull request and why?
- Mention config in `dbt_project.yml` (like for models, seeds, and
snapshots)
- Match the style for other resource types for
`models/semantic_models.yml`
- Clarify that `meta` is no longer "always" available as a top-level key
(see [#8766](dbt-labs/dbt-core#8766))

## Other

- Here's an open issue to add `meta` config for metrics:
[#3957](#3957)
- Opened an issue to add version blocks for any other resource types for
which it is applicable:
[#4224](#4224)
- Opened an issue to add code examples to [`meta`
](https://docs.getdbt.com/reference/resource-configs/meta) for
`dbt_project.yml` (like
[`enabled`](https://docs.getdbt.com/reference/resource-configs/enabled)
has them):
[#4415](#4415)


## 🎩 

### Previews
-
[1.6](https://docs-getdbt-com-git-dbeatty-semantic-models-met-b30fa1-dbt-labs.vercel.app/reference/resource-configs/meta?version=1.6)
-
[1.7](https://docs-getdbt-com-git-dbeatty-semantic-models-met-b30fa1-dbt-labs.vercel.app/reference/resource-configs/meta?version=1.7)

### v1.7

<img width="500" alt="image"
src="https://github.com/dbt-labs/docs.getdbt.com/assets/44704949/09de14e0-dd5b-40ef-8d9b-f1cd34a602a9">


## Checklist
- [x] I have tested that the code examples work
- [x] I have tested that the changes look correct in the Vercel preview
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
and [About
versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version)
so my content adheres to these guidelines.
@tlento tlento mentioned this pull request Feb 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QMalcolm QMalcolm QMalcolm approved these changes

@jzhu13 jzhu13 Awaiting requested review from jzhu13 jzhu13 is a code owner automatically assigned from dbt-labs/cloud-artifacts

@ChenyuLInx ChenyuLInx Awaiting requested review from ChenyuLInx ChenyuLInx is a code owner automatically assigned from dbt-labs/core-team

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emmyoop @dbeatty10 @QMalcolm