Merged
Conversation
- Mark pos1end as Required in segment geom - Leverage bidirectional validation to enforce "at least one of xend or yend" - Remove validation logic from SegmentRenderer, keep only transformation - Add 4 validation tests in segment.rs - Simplify SegmentRenderer to use entry().or_insert() pattern The bidirectional validation checks for either: - Identity: pos1, pos2, pos1end (x, y, xend) - Flipped: pos1, pos2, pos2end (y, x, yend) This effectively enforces "at least one endpoint required" without custom logic.
- Add GeomTrait::validate_aesthetics() method for custom validation - Mark pos1 as Required in rule geom - Implement validate_aesthetics() for Rule to enforce XOR (exactly one of x or y) - Call geom.validate_aesthetics() from Layer::validate_mapping() - Remove validation logic from RuleRenderer, keep only diagonal rendering - Add 3 validation tests in rule.rs Architecture improvement: - Geoms can now implement custom validation logic beyond Required/Null - Rule's XOR constraint (pos1 XOR pos2) enforced at geom layer - Writer only handles rendering, not validation
Move pos*min/pos*max to x/y/x2/y2 transformation from individual renderers to the central map_position_to_vegalite() function. This eliminates ad-hoc transformations scattered across ErrorBarRenderer, RibbonRenderer, and RectRenderer. Convention: min → primary channel (x/y), max → secondary channel (x2/y2) Changes: - Update map_position_to_vegalite() to map pos*min/pos*max directly to Vega-Lite encoding channels (x/y/x2/y2) - Remove entire RibbonRenderer (now uses DefaultRenderer) - Remove vestigial modify_encoding() from ErrorBarRenderer and RectRenderer - Remove 2 redundant tests (test_errorbar_encoding_transformation, test_rect_continuous_both_axes) - Update rect tests to reflect upstream transformation Result: 155 lines removed, cleaner separation between domain (aesthetics) and presentation (encoding channels). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Closed
Add coord_kind to RenderContext with a pre-computed PositionChannels tuple (pos1, pos1_end, pos1_offset, pos2, pos2_end, pos2_offset) that resolves to Vega-Lite channel names based on coordinate system. Replaces hardcoded "x"/"y"/"x2"/"y2" in 8 renderers (Bar, Path, Segment, Rule, Tile, Violin, ErrorBar, Boxplot) so they work with both Cartesian and polar coordinates. Also moves RenderContext from layer.rs to encoding.rs where it belongs, and adds &RenderContext to the finalize() trait method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ional geoms Add ErrorBar to geom_has_implicit_orientation so the orientation system correctly detects and flips horizontal errorbars. Fix global mapping merge to accept flipped position counterparts (e.g., pos2 into a geom declaring pos1) using flip_position, enabling global aesthetics like `species AS y` to reach bidirectional geoms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
georgestagg
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to fix #285 and fix #264 and, time permitting, also #310.
These unrelated issues are in the same PR because they touch the same code in the writer module and I wanted to avoid merge conflicts with myself.
If I can't finish #310 today, we can merge this as-is.