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

Standardize returning ResourceTypeSelector instances in dbt list and dbt build #10739

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Sep 18, 2024

Resolves NA

Problem

There is some unnecessary code duplication as it relates to ResourceTypeSelector. The backstory is explained in the PR description for #10718.

Solution

Standardize the duplicated code.

One wrinkle to confirm

The one thing that is technically different is this line that sets include_empty_nodes=True for dbt list. The include_empty_nodes=True line was introduced in 51da375 (within #7891), presumably to resolve #7496.

There is a single case where it is false here: when test is the only selected resource. That would be an odd carve out! So I'm assuming that it is unintentional and we can safely make the change in this PR.

However, if we do want this carve out, then we should create a functional test for it, because we don't have any for it currently.

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.
  • Tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.).
  • This PR includes type annotations for new and modified functions.

@dbeatty10 dbeatty10 added the Skip Changelog Skips GHA to check for changelog file label Sep 18, 2024
@cla-bot cla-bot bot added the cla:yes label Sep 18, 2024
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.95%. Comparing base (3e437a6) to head (092034e).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10739      +/-   ##
==========================================
+ Coverage   86.12%   88.95%   +2.83%     
==========================================
  Files         181      181              
  Lines       22964    22960       -4     
==========================================
+ Hits        19777    20425     +648     
+ Misses       3187     2535     -652     
Flag Coverage Δ
integration 86.13% <100.00%> (+0.01%) ⬆️
unit 62.41% <50.00%> (?)

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

Components Coverage Δ
Unit Tests 62.41% <50.00%> (∅)
Integration Tests 86.13% <100.00%> (+0.01%) ⬆️

@dbeatty10 dbeatty10 removed the Skip Changelog Skips GHA to check for changelog file label Sep 18, 2024
@dbeatty10 dbeatty10 marked this pull request as ready for review September 19, 2024 19:58
@dbeatty10 dbeatty10 requested a review from a team as a code owner September 19, 2024 19:58
@dbeatty10 dbeatty10 merged commit 7016cd3 into main Sep 19, 2024
66 of 67 checks passed
@dbeatty10 dbeatty10 deleted the dbeatty/tidy-first-10706 branch September 19, 2024 22:53
peterallenwebb pushed a commit that referenced this pull request Sep 20, 2024
…and `dbt build` (#10739)

* Remove duplicated constructor for `ResourceTypeSelector`

* Add type annotation for `ResourceTypeSelector`

* Standardize on constructor for `ResourceTypeSelector` where `include_empty_nodes=True`

* Changelog entry
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.

[CT-2517] Graph selection for public models from dependency projects
2 participants