Add table chart type - #528
Conversation
palewire
left a comment
There was a problem hiding this comment.
Thanks for taking on table support — this is a useful and important chart type, and I like the direction of giving users high-level objects for table columns, row styles, heatmaps, sparklines, and realistic Academy-derived examples.
I reviewed the PR diff, commits, docs/examples, implementation, test fixtures, and current PR status. I also prepared a fresh worktree and ran focused local checks against the PR head. My recommendation is request changes for now: the branch currently breaks package import/test collection, and there are several serialization/deserialization issues that would make the new API unreliable even after the import problem is fixed.
Blocking findings
-
The package cannot be imported.
datawrapper.charts.models.table_rowimportsTableTextStylefrom..models, whilemodels/__init__.pyis still importingtable_row, producing a circular import at import time. This blocks all tests and normal use of the library:ImportError: cannot import name 'TableTextStyle' from partially initialized module 'datawrapper.charts.models'Reproduced with:
uv run python - <<'PY' import datawrapper PY uv run pytest tests/integration/test_table.py -q uv run pytest tests/unit/test_enum_imports.py tests/unit/test_enum_backwards_compatibility.py -q
Both pytest invocations fail during
conftest.pyimport before collecting tests. This is the main release blocker. -
Table serialization writes the wrong value for compact mode.
InTable.serialize_model(),compactModeis set fromself.mobile_fallbackinstead ofself.compact_layout. That meanscompact_layout=Trueis ignored, andmobile_fallback=Trueaccidentally enables compact mode in the serialized API payload. -
Table deserialization does not round-trip two public fields because it uses the wrong Python names.
The model fields arecompact_layoutandparse_markdown, but deserialization setscompact_modeandmarkdown. Because the base model only warns on unrecognized fields, those API values are dropped and defaults are used. This matters for real Academy configs: the bundledlife_expectancy_heatmap.jsonhascompactMode: true, but this PR would deserialize it as the defaultcompact_layout=False. -
Heatmap deserialization crashes if a table has a
heatmapobject but nolegendobject.
legend_data = visualize.get("legend")can beNone, butHeatMap.deserialize_model()immediately callslegend_api_data.get(...). The fixture set happens to includelegend, but the method signature and API shape should tolerate absent legend metadata, especially because heatmap itself can be disabled. -
Heatmap legend serialization drops user-provided configuration.
HeatMapContinuous.serialize_model()replaces any non-false legend with a freshLegendContinuous()object, so a caller-provided title, size, labels, position, etc. is silently lost.Legend.serialize_model()also serializestitleas a one-element tuple (model["title"] = (self.title,)) rather than a string. These are easy to miss in tests because current assertions only checklegend.enabled. -
Column/row metadata serialization emits empty structures by default.
column_jsonandrow_jsonstart as{}, butif column_json is not None/if row_json is not Noneare always true, so even a basicTable(...)serializesmetadata.visualize.columns = {}androws = {}. That is different from comparable chart serializers in the repo, which generally add optional nested structures only when configured, and it makes round-trip comparisons noisier. -
There is debug/stdout output in library deserialization.
Table.deserialize_model()printstitle is ..., type is ...while grouping sparklines. Library code should not write to stdout during.get()or fixture parsing; tests can assert behavior without print statements. -
The documentation page is not linked into the Sphinx chart-type toctree, and one example has a syntax error.
docs/user-guide/charts/tables.mdis added, butdocs/index.mddoes not include it under Chart types. In the first example,source_url="https://data.worldbank.org/indicator/sp.dyn.le00.in"is missing a trailing comma beforedata=df, so the documented code does not run. -
The PR moves test-only tools into runtime dependencies.
pytest,pytest-cov,faker,mypy, andtypes-requestsare added to[project].dependencies, not just optionaltest/mypyextras. That expands the runtime install footprint for end users and is inconsistent with the existing optional-dependency layout. -
Current PR status is not enough to merge.
GitHub reports only the Read the Docs status check for this PR, and the PR is currently markedCONFLICTINGagainstmain. The repository workflow normally runs Ruff, mypy, tests across Python 3.10-3.13, and build; this branch needs to be rebased and pass that full workflow before release.
Realistic Academy fixture validation
I inspected the non-secret table fixtures added under tests/samples/table/, which appear to be realistic Datawrapper/Academy chart exports:
life_expectancy.json(563hg): 198 configured columns, 175 enabled sparkline columns, 6 row configs.life_expectancy_heatmap.json(FAawn): 63 configured columns, heatmap colors/custom stops,compactMode: true, 3 row configs.museums.json(7Jgac): 4 configured columns, flag replacement and bar-style table settings.unemployment.json(TRahm): 16 configured columns, enabled column heatmaps, custom heatmap colors/stops, 3 row configs.
Those are good sources and the PR is right to include them. However, the local Academy-config parsing/serialization tests could not be completed because the package import fails before Table.get() can run. I did verify statically that the fixtures exercise important fields that the current tests miss or do not fully assert, including compactMode, disabled top-level heatmaps with per-column heatmap settings, many sparkline columns, custom rows, and legend details.
Test strategy
The broad testing style is consistent with the project’s localized testing regime: chart-type-specific integration tests plus realistic sample JSON/CSV fixtures under tests/samples/<chart_type>/, similar to area/scatter/bar coverage. The problem is execution and depth: because import is currently broken, none of the new tests run; and after fixing import, the tests should add targeted assertions for the bugs above (compact mode, markdown aliasing, custom legend preservation, no stdout during deserialization, no empty optional columns/rows for basic charts, and docs examples that at least compile).
Smaller maintainability/API notes
Tabledocstring says “column chart,” likely copied from another class.TableTextStylehas a module-levelcolor = Field(...)and a module-level validator function that are not attached to the model; if the intent is to rejectcolor=True, that validator needs to live inside the class.bar_color_negativemixesbool | int | str; if this is intentional, the docs should clarify whenTruemaps to Datawrapper red versus a palette/hex value.TableMiniChart.typecould probably be narrowed to the existingLiterals at the base level or validated, since deserialization branches on it.- Consider exporting the new table helper classes from
datawrapper.charts.__init__as well as top-leveldatawrapper, if the intended import idiom is consistent with other chart-specific helper models.
Positive aspects
- The PR fills a real feature gap with a user-friendly API surface.
- The fixtures are valuable and grounded in real Datawrapper table configurations rather than toy-only cases.
- Pagination serializer, column style objects, row style objects, heatmap models, and mini chart grouping are the right general abstractions.
- The README-adjacent docs/examples are a good start once they are linked and made executable.
I would be happy to re-review after the import blocker, compact/markdown aliasing, heatmap/legend serialization, runtime dependency split, docs link/example syntax, and CI/rebase issues are addressed.
Implement Table chart class