refactor(diagrams): apply the large-graph layout policy at the render point - #10
Merged
Merged
Conversation
… point Follow-up to thomasmarsh#2, addressing the review: the helper had to be called by every builder, so a new diagram type would silently miss it. 'render_dot_to_svg' is already the codebase's single point for turning a graphviz object into SVG -- its docstring says every render must go through it, and it already carries one cross-cutting guarantee (a render failure must never break the embedding page). The size policy belongs in the same place, so this moves it there and drops the seven per-builder calls. Deciding centrally means the exceptions have to be explicit rather than implied by which builders remembered to opt in: * graphs that already chose a force-directed engine are left alone ('render_calls' uses fdp, 'render_heatmap' sfdp) * graphs whose ranks carry meaning opt out by name via RANKED_BY_MEANING -- currently the Hasse diagram, where the ranks are the partial order and a force-directed layout would destroy what it exists to show Also fixes a real bug in the merged version: it set overlap="prism", which is exactly the unsupported default 'render_dot_to_svg' warns about -- prism needs a triangulation library many graphviz builds omit, and would have failed the same way the docstring describes. Now overlap="false", matching 'render_calls'. Tests: the builder-level cases now exercise fit_layout_to_size directly, plus new cases for the two opt-outs and one asserting prism is never requested. FakeDot grows the attributes the render path now inspects. 26 passed against 23 on main for the same selection; the 17 errors are pre-existing here (fixtures shelling out to cabal).
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
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.
Follow-up to your review on #2 — you were right, and this is the version that does it once.
What changed
render_dot_to_svgis already the single place every graphviz object becomesSVG. Its docstring says so, and it already carries one cross-cutting guarantee
(a render failure must never break the embedding page). The size policy is the
same kind of concern, so it now lives there and the seven per-builder calls are
gone. A new diagram type gets it without knowing it exists.
The exceptions had to become explicit
Deciding centrally means the cases that should not be re-laid out can no
longer be implied by which builders remembered to opt in:
render_callsusesfdp,render_heatmapusessfdp. Checked via the engine, not a name list.RANKED_BY_MEANING. Currently justthe Hasse diagram, where the ranks are the partial order — force-directing
it would destroy the thing it exists to show. This is the one case that
genuinely needs a name, since "my ranks are semantic" is not observable from
the graph.
It also fixes a real bug I shipped in #2
The merged version sets
overlap="prism"— precisely the unsupported defaultrender_dot_to_svg's own docstring warns against:So on a graphviz build without that library, every large diagram would have
fallen back to the placeholder SVG. It is now
overlap="false", matchingrender_calls. There is a test asserting prism is never requested.Tests
Builder-level cases now exercise
fit_layout_to_sizedirectly rather thanasserting builders self-modify, plus new cases for both opt-outs and the prism
regression.
FakeDotgrows the attributes the render path now inspects, with acomment explaining why.
26 passed against 23 on
mainfor the same selection. The 17 errors in thatrun are pre-existing in my environment (fixtures shelling out to
cabal).Note
CI on this will fail in Set up GHC like everything else at the moment — #9 has
the diagnosis, and it is unrelated to any of these changes.