feat(annotations): enhance deserialization to handle list format and flatten position objects#508
Conversation
…flatten position objects - Update deserialize_model() to accept both dict and list input formats - Add position object flattening to extract x0/x1/y0/y1 (range) and x/y (text) fields - Improve handling of annotations from different sources (API vs serialization) - Add comprehensive test coverage for new deserialization scenarios This change makes the annotation models more flexible by supporting multiple input formats and automatically flattening nested position objects into their component coordinate fields, improving compatibility across different data sources.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances annotation model deserialization to handle multiple input formats (dict with UUID keys from API and list format from serialization) and automatically flattens nested position objects into top-level coordinate fields (x/y for text annotations, x0/x1/y0/y1 for range annotations). This improves compatibility across different data sources.
- Updated
deserialize_model()methods to accept bothdict[str, dict]andlist[dict]input formats - Added position object flattening to extract coordinate fields directly to the annotation dictionary
- Added comprehensive integration tests demonstrating real-world usage with scatter plot annotations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/models/test_range_annotation_permissive.py | Updated test assertions to reflect flattened position fields at top level |
| tests/samples/scatter/policy-poll.json | Added sample chart configuration with text annotations for integration testing |
| tests/samples/scatter/policy-poll.csv | Added sample CSV data corresponding to policy-poll.json |
| tests/integration/test_scatter_annotations.py | Added comprehensive integration tests for scatter plot annotation deserialization |
| datawrapper/charts/multiple_column.py | Updated deserialization to handle both dict and list formats for MultipleColumn annotations |
| datawrapper/charts/models/text_annotations.py | Updated deserialization with list format support and position flattening |
| datawrapper/charts/models/range_annotations.py | Updated deserialization with list format support and position flattening |
Comments suppressed due to low confidence (2)
datawrapper/charts/multiple_column.py:138
- The 'position' object is extracted from anno_data but never removed from anno_dict. This leaves the nested position object in the result alongside the flattened x/y fields, which is inconsistent with the behavior in text_annotations.py (line 329) and range_annotations.py (line 191) where position is removed with pop(). Add 'anno_dict.pop("position", None)' after line 125 to maintain consistency.
for anno_id, anno_data in items_to_process:
# Extract position data
position = anno_data.get("position", {})
x = position.get("x") if isinstance(position, dict) else None
y = position.get("y") if isinstance(position, dict) else None
plot = position.get("plot") if isinstance(position, dict) else None
# Extract showInAllPlots (defaults to False for text annotations)
show_in_all = anno_data.get("showInAllPlots", False)
# Build annotation dict with id
anno_dict = {**anno_data, "id": anno_id}
# Add position fields
if x is not None:
anno_dict["x"] = x
if y is not None:
anno_dict["y"] = y
# Add MultipleColumnChart-specific fields
if plot is not None:
anno_dict["plot"] = plot
anno_dict["show_in_all_plots"] = show_in_all
result.append(anno_dict)
datawrapper/charts/multiple_column.py:138
- Missing connector line handling that exists in the parent TextAnnotation.deserialize_model() (lines 334-341 in text_annotations.py). MultipleColumnTextAnnotation should handle connectorLine deserialization by checking if enabled is False and setting it to None, consistent with the parent class behavior.
for anno_id, anno_data in items_to_process:
# Extract position data
position = anno_data.get("position", {})
x = position.get("x") if isinstance(position, dict) else None
y = position.get("y") if isinstance(position, dict) else None
plot = position.get("plot") if isinstance(position, dict) else None
# Extract showInAllPlots (defaults to False for text annotations)
show_in_all = anno_data.get("showInAllPlots", False)
# Build annotation dict with id
anno_dict = {**anno_data, "id": anno_id}
# Add position fields
if x is not None:
anno_dict["x"] = x
if y is not None:
anno_dict["y"] = y
# Add MultipleColumnChart-specific fields
if plot is not None:
anno_dict["plot"] = plot
anno_dict["show_in_all_plots"] = show_in_all
result.append(anno_dict)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for anno_id, anno_data in items_to_process: | ||
| # Extract position data | ||
| position = anno_data.get("position", {}) | ||
| plot = position.get("plot") if isinstance(position, dict) else None |
There was a problem hiding this comment.
Similar to MultipleColumnTextAnnotation, the 'position' object should be removed from anno_dict after extracting fields to maintain consistency with range_annotations.py. The position object remains in the result dictionary alongside the flattened fields.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
This change makes the annotation models more flexible by supporting multiple input formats and automatically flattening nested position objects into their component coordinate fields, improving compatibility across different data sources.