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 default_granularity to metric spec #10377

Closed

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Jun 27, 2024

resolves #10376

Problem

Solution

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

@cla-bot cla-bot bot added the cla:yes label Jun 27, 2024
@courtneyholcomb courtneyholcomb added semantic Issues related to the semantic layer artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking labels Jun 27, 2024
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.16%. Comparing base (96b79a8) to head (f020cfb).

Additional details and impacted files
@@                        Coverage Diff                        @@
##           court/cumulative-type-params   #10377       +/-   ##
=================================================================
- Coverage                         88.77%   62.16%   -26.61%     
=================================================================
  Files                               180      180               
  Lines                             22517    22519        +2     
=================================================================
- Hits                              19989    14000     -5989     
- Misses                             2528     8519     +5991     
Flag Coverage Δ
integration ?
unit 62.16% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
Unit Tests 62.16% <100.00%> (+<0.01%) ⬆️
Integration Tests 62.16% <100.00%> (-26.61%) ⬇️

ChenyuLInx and others added 8 commits June 27, 2024 16:44
…flags (#10359)

* Fix `test_can_silence` tests in `test_warn_error_options.py` to ensure silencing

We're fixing an issue wherein `silence` specifications in the `dbt_project.yaml`
weren't being respected. This was odd since we had tests specifically for this.
It turns out the tests were broken. Essentially the warning was instead being raised
as an error due to `include: 'all'`. Then because it was being raised as an error,
the event wasn't going through the logger. We were only asserting in these tests that
the silenced event wasn't going through the logger (which it wasn't) so everything
"appeared" to be working. Unfortunately everything wasn't actually working. This is now
highlighted because `test_warn_error_options::TestWarnErrorOptionsFromProject:test_can_silence`
is now failing with this commit.

* Fix setting `warn_error_options` via `dbt_project.yaml` flags.

Back when I did the work for #10058 (specifically c52d653) I thought that
the `warn_error_options` would automatically be converted from the yaml
to the `WarnErrorOptions` object as we were building the `ProjectFlags` object,
which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought
this was validated by the `test_can_silence` test added in c52d653. However,
there were two problems:

1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a
`WarnErrorOptions` object
2. The `test_can_silence` test was broken, and not testing what I thought

The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions`
instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make
the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of
a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions`
definition to default `include` to an empty list. Doing so would allow us to get rid
of `convert_config` entirely.
@courtneyholcomb courtneyholcomb changed the base branch from main to court/cumulative-type-params June 27, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking cla:yes semantic Issues related to the semantic layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add time_granularity to metric spec
5 participants