Fix case sensitivity for grain_to_date and offset_to_grain
#370
Conversation
courtneyholcomb
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
courtneyholcomb
requested review from
aiguofer,
DevonFulcher and
theyostalservice
| if metric.type_params.grain_to_date and not metric.type_params.cumulative_type_params.grain_to_date: | ||
| metric.type_params.cumulative_type_params.grain_to_date = metric.type_params.grain_to_date.value | ||
|
|
||
| # Since grain_to_date is just a string, we can't add custom parsing to them. Instead, lowercase them here. |
There was a problem hiding this comment.
Actually... you can, I think? I might be missing some critical context here on how these values are processed, but you can override the parse_obj function in the Pydantic class that includes this string to change the behavior. (This took me an embarrassingly long time to figure out.)
I did this in #366 in dbt_semantic_interfaces/implementations/saved_query.py to differentiate between str and List[str].
There was a problem hiding this comment.
Good point! Silly me!
There was a problem hiding this comment.
Oh no please don't say that. Like I said, this took me way longer to figure out than I'd like to talk about, so if you're silly, then what am I?!?
| for semantic_model in model.semantic_models: | ||
| semantic_model.name = semantic_model.name.lower() | ||
| for metric in model.metrics: | ||
| metric.name = metric.name.lower() |
There was a problem hiding this comment.
This isn't one of the fixes mentioned in the description, is it?
| window: "7 Days" | ||
| """ | ||
| ) | ||
| for yaml_contents in (yaml_contents1, yaml_contents2): |
There was a problem hiding this comment.
Putting a for loop in to a test case to run multiple tests inside is generally an antipattern in test design. Test functions should be as simple as possible.
In this case, you could create two separate functions with a helper, or you could get fancy and use a data provider. (I think this is pytest, in which case, you might be able to use https://docs.pytest.org/en/stable/how-to/parametrize.html , but i say that without considering what version we use or anything else.)
| @@ -240,17 +263,35 @@ def test_grain_to_date_metric_parsing() -> None: | |||
| grain_to_date: "weEk" | |||
| """ | |||
| ) | |||
| file = YamlConfigFile(filepath="inline_for_test", contents=yaml_contents) | |||
| yaml_contents2 = textwrap.dedent( | |||
Description
In this PR, we unintentionally made
grain_to_dateandoffset_to_grainfields case sensitive. This needs to be fixed before we can release to core.Checklist
changie newto create a changelog entry