Use display names for aesthetics in stat error messages.#463
Use display names for aesthetics in stat error messages.#463teunbrand wants to merge 11 commits into
Conversation
Mappings now carries an optional AestheticContext field (set once during transform_to_internal). This eliminates the need to pass aesthetic_ctx as a separate parameter through ~30 stat/geom/execute function signatures. Added display_name() and internal_name() convenience methods on Mappings for translating between internal and user-facing aesthetic names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These functions now get context from the layer's Mappings directly, removing another set of explicit AestheticContext threading. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
add_discrete_columns_to_partition_by and flip_dataframe_position_columns now get AestheticContext from the layer's Mappings instead of requiring it as a separate parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The writer's validate_layer_columns now uses layer.mappings.display_name() instead of receiving a separate AestheticContext parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| /// Set the aesthetic context. Must only be called once. | ||
| pub fn set_context(&mut self, ctx: super::AestheticContext) { | ||
| debug_assert!(self.context.is_none(), "aesthetic context already set"); |
There was a problem hiding this comment.
This gives a sort of 'soft immutability' to the context field, which helps preserving its role as a source of thruth.
|
I am going to push back on this for the reason that we might end up with a dynamic AestheticContext due to interactivity. I'm not 100% sure it will materialise but it feels wrong and premature to do this work now before we have figured it out |
|
Is the objection against (1) the 'soft immutability' or (2) against bundling mapping with aesthetic context? If (1), I'd argue that this is just a 1 LOC guard that we can relieve once the need arises. |
2 participants
This PR aims to fix #435.
The main change is that the
Mappingsstruct now has acontextfield that optionally holds anAestheticContext. The benefit it twofold:The only downside is that we move from a single plot-wide source of truth about AestheticContext, to layer-level sources of truth. This is more of a theoretical than practical objection though and I think it is worth it.