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 support for File Selectors and add file selectors to the default method selector list #5241

Merged
merged 3 commits into from
May 13, 2022

Conversation

jwills
Copy link
Contributor

@jwills jwills commented May 13, 2022

resolves this small gripe: "To this day, I can’t give dbt a file name and hope it figures out what I mean, but instead I still have to remove the .sql at the end."

https://pedram.substack.com/p/we-need-to-talk-about-dbt

and thus fix #5240

Description

I'm adding a FileSelectorMethod that uses the name of a .sql file as the way to find model(s) to run via a selector and I added a check to the default_method code that does an automatic determination of which selector method to use that will prefer the File selector to the FQN selector for selector values that end with the string ".sql".

Checklist

Josh Wills added 2 commits May 12, 2022 21:21
…selection criteria if the given selector has a . in it but no path separators
@jwills jwills requested a review from a team May 13, 2022 04:57
@jwills jwills requested a review from a team as a code owner May 13, 2022 04:57
@jwills jwills requested review from ChenyuLInx and stu-k May 13, 2022 04:57
@cla-bot cla-bot bot added the cla:yes label May 13, 2022
@jwills
Copy link
Contributor Author

jwills commented May 13, 2022

Hi! So this is a, uhh, emotionally fraught (?!) pull request, but I don't want to let that get in the way of, ya know, writing good software.

The other way I could have implemented this feature was by removing the last few characters of the input selector if they looked like a file extension (that is, .sql) and then falling back to the FQN method. I didn't have a strong preference between what I did here and that option; I'm not super-familiar with the code I'm changing here and so I went with the new file selector impl on the principle that I should try not to muck with user input too much, but am more than happy to take this code in a different direction if there are other considerations here that I am unaware of. 🙇

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

@jwills Thanks for submitting this so fast!!! And take me to different parts of the codebase! I think the current approach looks great!
If we take the other route you mentioned, we will run into some unintended behavior. For example, in my models dir I have,

test.sql
test/inside.sql

If I do dbt ls -s test.sql it will actually show both.
Maybe there's a path to refactor but I think what we currently get looks great!

@ChenyuLInx ChenyuLInx merged commit 03b17ff into dbt-labs:main May 13, 2022
@jwills jwills deleted the jwills_try_harder_to_find_the_models branch May 13, 2022 17:41
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
…method selector list (dbt-labs#5241)

* Add a new selector method for files and add it to the default method selection criteria if the given selector has a . in it but no path separators

* Add a file: selector method to the default selector methods because it will make Pedram happy

* changie stuff
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-640] File selectors and support for file selectors as a default method
3 participants