Repository navigation
nixoslogo: architecture refactor phase 1 - #79
Merged
Merged
Conversation
Design spec for restructuring the nixoslogo package: keyword-only dataclasses with a lazy canvas, composition where inheritance was wrong, immutable annotation presets with typed overrides, and a font pipeline on fontTools and HarfBuzz that reads each font once into immutable outlines. Records the ten migration steps, which artifacts change bytes and by how much, and how each step is verified. Assisted-by: Claude Code (claude-fable-5-1)
Corrections from an adversarial pass with implementation attempts on scratch copies: subclasses forward clear_space to the base in step 1, the dimensioned overlay step moves ahead of the dataclass step because of field invariance, optional-input fields get a resolution rule, uharfbuzz needs a local stub to pass basedpyright, CFF composites are decomposed before contour reversal, the label groups on the dimensioned logotype move with their text, and the numeric comparison pairs numbers per aligned element. The tight-versus-control box note now names the wordmark "i" as the one affected glyph. Assisted-by: Claude Code (claude-fable-5-1)
Seven byte-identical steps from the design spec as bite-sized tasks: a fast harness over the 96 artifact scripts, the lazy canvas and clear space rule, lambda overlay functions, keyword-only dataclasses, frozen annotation presets with typed overrides, the two preset misuse classes removed, LogoClearspace over an instance, and numeric and raster modes for compare-artifacts. Each task has its failing test, its exact edits, the four gates and its commit. Assisted-by: Claude Code (claude-fable-5-1)
Three reviewers executed the plan literally on scratch copies and found: the harness compared by directory name while seven scripts write a different file name; three more local variables shadow the new hexagon_points function; ClearSpace imports go unused after the dataclass step; the registry ruff is newer than the flake-pinned one the formatter runs; the clearspace diagram's helper kept a BaseRenderable parameter; and the numeric comparison measured the parser's own structural tokens. All fixed, plus one test expectation that was wrong and a README sentence left stale. Assisted-by: Claude Code (claude-fable-5-1)
The lazy-canvas test could not fail, because it compared the property with the call it is defined as; it now changes clear_space after construction and checks the canvas follows. The generalised spacing dimensions gain a three-glyph test, the only check for a length other than the five the artifacts cover. TextAnnotations drops original_scale, which was assigned and never read, and the scale input that nothing reaches once make_annotation is the only caller. Assisted-by: Claude Code (claude-fable-5-1)
Sixteen of the eighteen package tests failed only on things that change bytes in a shipped artifact, which the harness and the build catch first, or asserted language guarantees such as a frozen dataclass rejecting assignment. The one test left in the package pins the generalised spacing dimensions at a glyph count the artifacts never run. Two trivial comparison-tool tests go for the same reason. Review Focus now names the artifact that pins each failure mode. Assisted-by: Claude Code (claude-fable-5-1)
The spacing-dimensions test guarded a glyph count nothing ships; the five-glyph artifact already pins the only caller. With no package tests left in this phase, Task 0 no longer adds pytest to the devshell and becomes the harness alone. Phase 2 adds pytest with the font pipeline tests the artifacts cannot observe. Assisted-by: Claude Code (claude-fable-5-1)
colors_name, colors_value and css_color_names are pure functions of the colors and color_style fields, and cap_height and scale are the loader's own values, so Task 3 makes them properties instead of copies set in __post_init__. Logomark then has no __post_init__ for its subclasses to chain. Same values, same bytes. Assisted-by: Claude Code (claude-fable-5-1)
sized, small, medium and large build through cls, so Self is the accurate return type; the quoted class name was carried over from today's code. geometry.py keeps concrete names on purpose, because its operators return fixed classes. Assisted-by: Claude Code (claude-fable-5-1)
Matches the plan: the presets build through cls, so Self is the accurate type. Assisted-by: Claude Code (claude-fable-5-1)
max_delta alone cannot tell a whole-file drift from one moved coordinate. NumericResult gains compared (numbers on matched lines) and changed (how many differ), both free in the existing loop, and the CLI line and README carry them. Assisted-by: Claude Code (claude-fable-5-1)
The depth-tag and inserted-element tests now check compared and changed too, so a renumbered tag counting as a number would fail. Assisted-by: Claude Code (claude-fable-5-1)
Every environment the plan runs them in provides resvg, so the guard never fires, and if it did it would hide three skipped tests as a pass. A missing binary should fail. Assisted-by: Claude Code (claude-fable-5-1)
BaseRenderable no longer calls the virtual _make_canvas during construction; canvas is a property over make_canvas(clear_space), so the order of attribute assignments in subclasses stops mattering. The five identical _get_clearspace match blocks become ClearSpace.distance with a clearspace_unit per class. Shape holds the geometry surface that parents consume; BaseRenderable adds the document behaviour. Subclasses forward clear_space to the base instead of assigning it themselves. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
The construction lines, diagonals and outline polygons of the lambda diagrams were methods on DimensionedLambda, which forced the logomark diagrams to require a DimensionedLambda and to read line styles through it. They are module functions taking a Lambda and a LineGroup now, and the logomark diagrams take any Lambda and read their own annotations. make_hexagon_points never read self and is hexagon_points. The logotype spacing dimensions derive their count from the glyphs. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
Fields are the constructor parameters; values that are pure functions of them are properties, and the glyph runs that must be built once are set in __post_init__. An unknown keyword raises TypeError instead of disappearing into **kwargs, every Logomark gets its own Lambda, and subclasses redeclare the defaults they change. NixosLogo loses the loader parameter that positioned the logotype with the wrong cap height and reads the cap height back from the logotype it built. FontConfig loses stroke_dasharray, which only ever survived because the **kwargs hole swallowed it. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
LineGroup, DimensionLines and Annotations are frozen dataclasses. The three presets are one constructor, sized(multiplier), that reproduces their literals, and it takes a typed override dictionary in place of the attribute assignments six scripts made after construction. The LineConfig and FontConfig TypedDicts and the empty ObjectLines and ConstructionLines subclasses are gone. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
LogoScale and LogoColorsWithLambdaPrime changed a default and a filename and nothing else. Their scripts call NixosLogo with those arguments and pass the filename to write_svg. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
LogoClearspace took a class and instantiated it twice to obtain two canvases that differ by one number. It takes an instance and derives both rectangles from make_canvas, so the three scripts build the logo themselves and the font is loaded once per diagram. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
--numeric aligns the number-masked lines of each modified artifact and reports the maximum delta over aligned lines and the count of lines that did not align. The parser's own depth tags and index labels are masked for alignment but excluded from measurement, so an inserted sibling renumbering later ones is counted, not measured. --raster renders both versions with resvg and reports differing pixels. Both are written into the summary JSON only when requested; the four count keys are unchanged otherwise. Assisted-by: Claude Code (claude-fable-5-1)
dimension_spacings built the side list as ["left"] plus len(gaps) - 1 copies of "right", which is one element long for a single glyph and made the strict zip raise ValueError. The comprehension yields one side per gap, so a one-glyph DimensionedLogotype draws its cap height and bearing with no spacing dimensions. Five glyphs produce the same list as before. All 96 artifacts are byte-identical. Assisted-by: Claude Code (claude-fable-5-1)
Section 3.2 said font, glyph and layer are annotated with the fontforge classes until step 9 removes them. The plan chose plain attributes because fontforge ships no stubs, and the implementation follows the plan. The spec now says the same so the Phase 2 plan does not look for annotations that were never added. Assisted-by: Claude Code (claude-fable-5-1)
Artifact comparison0 modified · 0 added · 0 deleted · 96 unchanged Download report — HTML file; open in a browser. Compared
|
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.
Phase 1 of the nixoslogo architecture refactor. Every one of the 96 shipped SVG artifacts is byte-identical after every commit; no
outputHashchanges.What changes in
nixoslogo:Shapeholds the geometry surface (elements_bounding_box,make_svg_elements, theelements_*properties);BaseRenderableadds the document behaviour. The canvas is a lazy property overmake_canvas(clear_space), so construction order in subclasses stops mattering, and the five identical_get_clearspacematch blocks becomeClearSpace.distanceplus aclearspace_unitper class.Lambdaand aLineGroup. The logomark diagrams take anyLambdaand read their own annotations instead of requiring aDimensionedLambda.dimension_spacingsderives its count from the glyphs.@dataclass(kw_only=True). Fields are the constructor parameters, derived state lives in__post_init__or properties, unknown keywords raiseTypeErrorinstead of vanishing into**kwargs, everyLogomarkgets its ownLambda, andNixosLogoloses theloaderparameter that positioned the logotype with the wrong cap height.LineGroup,DimensionLinesandAnnotationsare frozen. The three presets aresized(multiplier)with a typed override dictionary replacing the attribute assignments six scripts made after construction.FontConfigandLineConfigare gone.LogoScaleandLogoColorsWithLambdaPrimeare deleted; their scripts callNixosLogowith the same arguments and pass the filename explicitly.LogoClearspacetakes a logo instance and derives both clear-space rectangles frommake_canvas, so the font loads once per diagram.What changes in
compare-artifacts:--numericaligns the number-masked lines of each modified artifact with difflib and reports matched and unmatched line counts, the numbers compared and changed on matched lines, and the maximum delta. The parser's depth tags and index labels are masked for alignment but not measured, so an inserted sibling is counted rather than reported as a huge positional delta.--rasterrenders both versions with resvg and reports differing pixels and the maximum channel delta.--summaryJSON only when requested; the four count keys are unchanged otherwise. Pillow is a runtime dependency and resvg is on the wrapper PATH. Six tests cover the two modes.The design and the implementation plan are under
docs/superpowers/. Phases 2 (fontTools and HarfBuzz pipeline, with the planned hash regeneration) and 3 (NixosLogocomposed from a logomark and a logotype) follow in their own PRs.Verification at the branch head:
nix build .#hydraJobs.nixos-branding.x86_64-linux.all-artifactssucceeds and its output is identical to themainbuild underdiff -rq; a local harness running all 96 artifact scripts reports 96 identical; basedpyright at 0 errors; ruff and treefmt clean; the compare-artifacts package build runs 62 tests.Assisted-by: Claude Code (claude-fable-5-1)