Conversation
|
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. |
* Use nouns for fields - `fill_nulls_with` -> `null_fill_value` * Update the type for the metric null fill value to allow for floats. * Rename MetricInput.as_reference -> MetricInput.metric_reference * `timespine` -> `time_spine` in variable names since it's 2 words.
plypaul
force-pushed
the
plypaul--53--assorted-nits
branch
from
e29b0e6 to
0d5d374
Compare
|
@plypaul this is currently marked as a draft. Are we trying to get this into into 0.4.0 of DSI? |
plypaul
requested review from
a team,
QMalcolm and
graciegoheen
as code owners
|
@QMalcolm - Picking this back up again. What would be the best way forward on this? |
tlento
left a comment
tlento
left a comment
There was a problem hiding this comment.
None of the original blockers on this PR have changed, but for whatever reason we didn't write them down, so now I'm writing them down.
| filter: Optional[PydanticWhereFilterIntersection] = None | ||
| alias: Optional[str] = None | ||
| time_spine_join: bool = False | ||
| null_fill_value: Optional[float] = None |
There was a problem hiding this comment.
When we tried this float caused problems when the actual value is supposed to be an int.
In a perfect world we could do int | float but that doesn't appear to resolve correctly and we haven't figured out how to deal with it.
| def time_spine_join(self) -> bool: | ||
| """If the measure should be joined to the time spine.""" | ||
| pass | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def fill_nulls_with(self) -> Optional[int]: | ||
| def null_fill_value(self) -> Optional[float]: |
There was a problem hiding this comment.
These are hard breaking changes on the dbt-core parser so we can't push these into dbt-semantic-interfaces or take dependencies on these names in MetricFlow if we want to be able to backport things.
It's fine to add duplicate fields with the new names, and once we get support for both broadly available across the releases we are committed to supporting we can gradually remove the old ones, but given that 1) these names were chosen within the context of existing dbt metrics nomenclature and 2) they're now out in the world I'm not sure this adjustment is worth the effort.
3 participants
Description
fill_nulls_with->null_fill_valuetimespine->time_spinein field names since it's 2 words.Checklist
changie newto create a changelog entry