Skip to content

Add table chart type - #528

Open
kellyjfitz wants to merge 2 commits into
chekos:mainfrom
kellyjfitz:add-table-chart-type
Open

Add table chart type#528
kellyjfitz wants to merge 2 commits into
chekos:mainfrom
kellyjfitz:add-table-chart-type

Conversation

@kellyjfitz

Copy link
Copy Markdown
Contributor

Implement Table chart class

  • Introduce a Table class
  • Supports heatmaps in table columns
  • Supports heatmap legend
  • Supports bar charts in table columns
  • Supports mini column and mini line charts in columns
  • Supports coloring table cells by category

@palewire palewire left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The package cannot be imported.
    datawrapper.charts.models.table_row imports TableTextStyle from ..models, while models/__init__.py is still importing table_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.py import before collecting tests. This is the main release blocker.

  2. Table serialization writes the wrong value for compact mode.
    In Table.serialize_model(), compactMode is set from self.mobile_fallback instead of self.compact_layout. That means compact_layout=True is ignored, and mobile_fallback=True accidentally enables compact mode in the serialized API payload.

  3. Table deserialization does not round-trip two public fields because it uses the wrong Python names.
    The model fields are compact_layout and parse_markdown, but deserialization sets compact_mode and markdown. 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 bundled life_expectancy_heatmap.json has compactMode: true, but this PR would deserialize it as the default compact_layout=False.

  4. Heatmap deserialization crashes if a table has a heatmap object but no legend object.
    legend_data = visualize.get("legend") can be None, but HeatMap.deserialize_model() immediately calls legend_api_data.get(...). The fixture set happens to include legend, but the method signature and API shape should tolerate absent legend metadata, especially because heatmap itself can be disabled.

  5. Heatmap legend serialization drops user-provided configuration.
    HeatMapContinuous.serialize_model() replaces any non-false legend with a fresh LegendContinuous() object, so a caller-provided title, size, labels, position, etc. is silently lost. Legend.serialize_model() also serializes title as a one-element tuple (model["title"] = (self.title,)) rather than a string. These are easy to miss in tests because current assertions only check legend.enabled.

  6. Column/row metadata serialization emits empty structures by default.
    column_json and row_json start as {}, but if column_json is not None / if row_json is not None are always true, so even a basic Table(...) serializes metadata.visualize.columns = {} and rows = {}. 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.

  7. There is debug/stdout output in library deserialization.
    Table.deserialize_model() prints title 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.

  8. The documentation page is not linked into the Sphinx chart-type toctree, and one example has a syntax error.
    docs/user-guide/charts/tables.md is added, but docs/index.md does 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 before data=df, so the documented code does not run.

  9. The PR moves test-only tools into runtime dependencies.
    pytest, pytest-cov, faker, mypy, and types-requests are added to [project].dependencies, not just optional test/mypy extras. That expands the runtime install footprint for end users and is inconsistent with the existing optional-dependency layout.

  10. 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 marked CONFLICTING against main. 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

  • Table docstring says “column chart,” likely copied from another class.
  • TableTextStyle has a module-level color = Field(...) and a module-level validator function that are not attached to the model; if the intent is to reject color=True, that validator needs to live inside the class.
  • bar_color_negative mixes bool | int | str; if this is intentional, the docs should clarify when True maps to Datawrapper red versus a palette/hex value.
  • TableMiniChart.type could probably be narrowed to the existing Literals 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-level datawrapper, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants