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

When parsing 'all_sources' should be a list of unique dirs #5176

Merged
merged 6 commits into from
Apr 28, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Apr 27, 2022

resolves #5120

Description

When two (or more) of the "-paths" settings point to the same directory, the "all_sources" method will return duplicate directory names, causing the attempt to parse the same files twice.

Checklist

@gshank gshank requested a review from a team as a code owner April 27, 2022 14:25
@cla-bot cla-bot bot added the cla:yes label Apr 27, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@gshank gshank requested a review from a team as a code owner April 27, 2022 14:27
@gshank gshank requested a review from emmyoop April 27, 2022 14:27
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

nice and simple way to dedup the list. I do want to point out that it will change the order of the final list since Sets are unordered, so as long as that's not an issue this should be fine.

@gshank gshank merged commit f633e99 into main Apr 28, 2022
@gshank gshank deleted the ct-524-unique_dirs_in_all_sources branch April 28, 2022 01:02
@emmyoop
Copy link
Member

emmyoop commented May 13, 2022

@leahwicz or @jtcohen6 should this be backported to 1.1.latest for the next patch?

@leahwicz
Copy link
Contributor

I would be ok backporting this since its a bug and it's a minimal change (most changes are involving test conversion)

@gshank
Copy link
Contributor Author

gshank commented May 16, 2022

Yeah, it's only a couple of lines. I'll backport without the tests.

github-actions bot pushed a commit that referenced this pull request May 16, 2022
* When parsing 'all_sources' should be a list of unique dirs

* Changie

* Fix some unit tests of all_source_paths

* Convert 039_config_tests

* Remove old 039_config_tests

* Add test for duplicate directories in 'all_source_files'

(cherry picked from commit f633e99)
gshank added a commit that referenced this pull request May 16, 2022
…5256)

* When parsing 'all_sources' should be a list of unique dirs

* Changie

* Fix some unit tests of all_source_paths

* Convert 039_config_tests

* Remove old 039_config_tests

* Add test for duplicate directories in 'all_source_files'

(cherry picked from commit f633e99)

Co-authored-by: Gerda Shank <[email protected]>
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
…5176)

* When parsing 'all_sources' should be a list of unique dirs

* Changie

* Fix some unit tests of all_source_paths

* Convert 039_config_tests

* Remove old 039_config_tests

* Add test for duplicate directories in 'all_source_files'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-524] [Bug] Issue co-locating models and seeds in the same directory
4 participants