Merge semantic model meta config #367
Conversation
DevonFulcher
requested review from
courtneyholcomb,
WilliamDee and
dbeatty10
DevonFulcher
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
|
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. |
There was a problem hiding this comment.
@DevonFulcher I couldn't tell if the tests are verifying that meta is "merging" rather than "clobbering".
Is there a way to add to the YAML contents and assertions of test case so that we know that it is merging rather than clobbering? I'm thinking a couple small additions could help differentiate.
| assert semantic_model.dimensions[3].config.meta["dim_metadata"] == "gfds" | ||
| assert semantic_model.dimensions[3].config.meta["sm_metadata"] == "asdf" |
There was a problem hiding this comment.
These lines cover the test cases I was thinking about here: #367 (review) ✅
Namely, it verifies that it merged the two dictionaries and has the sm_metadata key/value inherited from the semantic model as well as the dim_metadata key/value defined on the dimension.
There was a problem hiding this comment.
Per discussion with @DevonFulcher, we decided that enumerating similar test cases for entities and measures would be overkill because they are covered by the tests for dimensions.
The reason is that dimensions, entities, and measures are all treated the same here:
elements: Sequence[Union[PydanticDimension, PydanticEntity, PydanticMeasure]] = [
*sm.dimensions,
*sm.entities,
*sm.measures,
Description
This PR implements the meta config merge behavior described here and referenced here.
I decided to stack this PR on top to keep it distinct from the other PRs, but I don't intend to merge this one into main directly. I will merge it into its parent branch first.
Checklist
I have runI'm not going to merge this directly into main, so we don't need a separate changelog entry.changie newto create a changelog entry