Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Updating to match current file path naming schemes. #29

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Oct 22, 2021

making changes to match dbt-core test -> tests naming scheme and updating seeds-path to yaml files.

Issues: #dbt-labs/dbt-core#2659

test/<rest of path> -> tests/<rest of path>

fixes issues in files data_test_ephemerals.ym, and data_tests.yml to match dbt-core file naming scheme from test -> tests.

TO BE USED FOR versions 1.0.01b and up

-- Co-Author Kyle Wigley

@jtcohen6
Copy link
Contributor

@McKnight-42 Thanks for catching the test/ --> tests/ rename! I missed that in #28.

Any reason to prefer data/ + specifying seed-paths, rather than just keeping the renamed configs as seeds/?

@@ -1,14 +1,15 @@
name: base
paths:
seeds/base.csv: files.seeds.base
data/base.csv: files.seeds.base

Choose a reason for hiding this comment

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

Didn't we go from data to seeds? CC @jtcohen6

Copy link
Contributor

@jtcohen6 jtcohen6 Oct 26, 2021

Choose a reason for hiding this comment

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

We did. I guess this works because Matt's also setting seed-paths: ['data'] below—that is, overriding the default value (seeds/) to keep using the old name (data/) instead. I'm wondering if there's a reason to prefer doing it that way, rather than just using the new default name (seeds/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can change it back, we tried both, and I think got swapped around which was the newer of the name changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it back to seeds @jtcohen6 if you could please take a look and see if there are any other changes you think would make sense. please let me know.

rowcount: 10
added:
rowcount: 20
seed:

Choose a reason for hiding this comment

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

I think you have a spacing issue (tabs/spaces maybe) where it it shows up as something is difference. Let's look at cleaning that up before merging. I can help tomorrow

Copy link
Contributor

@kwigley kwigley Oct 27, 2021

Choose a reason for hiding this comment

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

@leahwicz I think these spacing changes are fine for now. It makes this file consistent with other yaml files 👍 in this directory (other files use 4 space indent). We should probably stick to the default 2 spaces for all yaml files, but prefer consistency for now.

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

lgtm! @leahwicz, can you confirm this works with dbt-spark before merging?

@@ -8,7 +8,7 @@ paths:
dbt_project_yml:
models:
dbt_test_project:

Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove these spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there, i believe latest commit does

rowcount: 10
added:
rowcount: 20
seed:
Copy link
Contributor

@kwigley kwigley Oct 27, 2021

Choose a reason for hiding this comment

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

@leahwicz I think these spacing changes are fine for now. It makes this file consistent with other yaml files 👍 in this directory (other files use 4 space indent). We should probably stick to the default 2 spaces for all yaml files, but prefer consistency for now.

@leahwicz
Copy link

Yep we are good to go here. Thanks @McKnight-42

@McKnight-42 McKnight-42 merged commit 29356d9 into master Oct 27, 2021
@kwigley kwigley deleted the mattmcknight/update-tests-path branch October 28, 2021 13:21
@jtcohen6 jtcohen6 mentioned this pull request Nov 9, 2021
@jtcohen6 jtcohen6 mentioned this pull request Nov 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants