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. |
|
Against 2. Distributing immutable Context's around the codebase makes it much harder to update dynamically when we hit that. So, I'd rather we do the boring thing and string it through to the call sites that need it |
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.