use MethodName.File when value ends with .py #5295
Conversation
|
@leoebfolsom the changes and test look great to me! We are going to hold off on merging this until the python model is put out into the main branch. |
core/dbt/graph/selector_spec.py
Outdated
| @@ -80,7 +80,7 @@ def __post_init__(self): | |||
| def default_method(cls, value: str) -> MethodName: | |||
| if _probably_path(value): | |||
| return MethodName.Path | |||
| elif value.lower().endswith(".sql"): | |||
| elif value.lower().endswith((".sql",".py")): | |||
There was a problem hiding this comment.
(Not to expand the scope of this well-defined and narrowly targeted PR, but: should we also have ".csv" here?)
There was a problem hiding this comment.
I'll defer to @jtcohen6 and @ChenyuLInx on this decision. I'd have to review the dependencies / code in more detail to understand whether including .csv would require additional changes, and I'm sure you have a better sense for that. If it's simply a matter of adding the .csv to this one line to slightly expand this PR, I definitely don't object! ty
|
I think we're finally ready for this one :) @ChenyuLInx |
Haha I was looking for this today then saw your comment |
| @@ -21,6 +22,7 @@ def models(self): | |||
| return { | |||
| "view_model.sql": base_view_sql, | |||
| "table_model.sql": base_table_sql, | |||
| "table_model.py": base_table_py, | |||
There was a problem hiding this comment.
I think we probably don't want to add this to the base adapter tests as not all adapters support running this
There was a problem hiding this comment.
Just the unittest above looks great to me
There was a problem hiding this comment.
All right, and it looks like you made that change, so you don't need anything from me?
There was a problem hiding this comment.
Just saw your other comment--looks good to me! Thank you!
There was a problem hiding this comment.
Sorry I just went ahead and did it thinking that it would be great for this to be included in the Beta release coming up soon!
Thank you so much for contributing this and the super quick response!!!
|
@leoebfolsom we are back since PR for python model just merged. I am going to just remove the changes in integration test and just push this in. Let me know if there's anything else you would want us to update! |
|
Close and reopen to rerun synk check |
Resolves #5289
Description
This allows the user to write a command such as
dbt run -m my_fancy_stats_model.py. Currently, a model ending in.py(acknowledging such models don't yet exist) cannot include the file extension in thedbt run -mcommand.My approach here was to create a new fixture called
table_model_py. This allowed me to not touchtable_modelbut still achieve the desired functionality. I thought this was a reasonable first step, in the spirit of "optimize later."However, I think a cleaner approach would be either:
a) Updating
table_modelto allow for both.sqland.pyextensions; orb) Rename
table_modelto betable_model_sql, and maintain it alongsidetable_model_py.I'm guessing
awould be preferred (more scalable, less repeated code and less code in general), but I didn't want to build something like that out without getting input from others.This is my first (attempted) contribution to dbt-core, so I may be doing things wrong--comments/guidance are welcome and desired! Thank you.
Checklist
changie newto create a changelog entry