Merge pull request 'Draw the Hirst painting (MIL-002)' (#16) from mil-002-spot-painting into main

Reviewed-on: #16

docs/artifact-registry.md

@@ -15,7 +15,7 @@ document of a type. `Primary File` may contain a glob (e.g.
 | SA | Stakeholder Analysis | docs/stakeholder-analysis.md | 002 |
 | PP | Project Plan | docs/project-plan.md | 002 |
 | MIL | Milestone / Gateway | docs/milestones/*.md | 003 |
-| RC | SQA Review Record | docs/sqa/reviews/rc-*.md | 006 |
+| RC | SQA Review Record | docs/sqa/reviews/rc-*.md | 007 |
 
 ## Languages
 

docs/sqa/reviews/rc-006-code-mil-002.md

@@ -0,0 +1,79 @@
+# Review Record: Code of MIL-002 (Spot Painting)
+
+## Metadata
+| Key | Value |
+| --- | --- |
+| ID | RC-006 |
+| CrossReference | [MIL-002], [MIL-001], [BC-001], [QC-PY-001] |
+
+## Version History
+| Date | Status | Author | Reviewer | Change | Commit |
+| --- | --- | --- | --- | --- | --- |
+| 2026-10-08 | Accepted | Jens Tirsvad Nielsen | S01 | Initial version (draft prepared by the assistant for S01 to confirm)<br>Verdict `Go` confirmed by S01 | [c3cfb64] |
+
+---
+
+## Artifact Under Review
+
+- Instance reviewed: the code of [MIL-002] tasks 1 to 6 (issues #7 to #12): the additions to `src/hirst_painting.py` (`create_pen`, `dot_positions`, `DotPen`, `draw_dots`, `main` and their constants) and the 22 new tests in `tests/test_hirst_painting.py` (40 tests in all, 18 of them from [MIL-001]). The tool configuration in `pyproject.toml` is unchanged from the version merged with [MIL-001]
+- Checklist used: [QC-PY-001]
+- Scope: full review of the MIL-002 additions, as they stand in the working tree of branch `mil-002-spot-painting` on 2026-10-08 (not yet committed). A change to the code before it is committed needs a delta re-review
+- Language and domain: n/a (source code is a technical type, always IT Professional English)
+- Language reviewer: none. S01 is also the author of record; S01 accepted a documented self-review on 2026-10-07 (see the review record of BC-001, Action Item 1)
+
+**Status of this record:** final. The assistant read the code against every
+criterion and ran the checks named below; S01 confirmed the verdict `Go` on
+2026-10-08.
+
+Evidence gathered on 2026-10-08 with Python 3.13.14, `ruff` 0.16.10, `mypy`
+2.4.0 and `pytest` 9.1.1:
+
+- `ruff format --check .` reports no changes and `ruff check .` reports "All checks passed!"
+- `mypy` (strict, set in `pyproject.toml`) reports "no issues found in 2 source files"
+- `pytest` reports 40 passed; the same 40 pass in reverse order and each passes when run alone; eight full runs in a row passed after the Tk start-up fix described under criterion 11
+- A search of `src/` and `tests/` for `print(`, `logging`, `except`, `noqa` and `nosec` finds nothing; the only `type: ignore` is explained (criterion 3)
+- A syntax-tree scan finds every function in `src/` annotated and documented, no `Any`, and no mutable or call-valued default argument. It lists one name that is also a module attribute of `builtins`, `__init__`, which is the constructor of the test fake `RecordingPen`, not a shadowed builtin
+- 19 broken copies of the MIL-002 code (mutants), each run with the reference image beside it, each failed the intended tests: the pen setup (no `hideturtle`, no `penup`, no `colormode`), the geometry (spacing 49, 9 rows, 11 columns, grid not centred), the dots (size 21, colour chooser ignored, no move, no empty-palette check), `main` (no `exitonclick`, wrong palette, click wait before drawing), and the speed-up (no `tracer(0)`, `tracer(1)`, no `update`, `update` after the click wait, `update` and click wait swapped)
+- A real run of `main()` on a real window drew the painting in 0.35 seconds in three runs, stayed open, and closed on a click; a captured picture shows 100 dots in a centred 10 by 10 grid with no trail and no turtle cursor
+
+## Checklist Results
+
+| # | Criterion | Status | Evidence/Notes |
+| --- | --- | --- | --- |
+| 1 | Packages, modules, functions, variables, classes and constants follow PEP 8 casing (`snake_case`, `PascalCase`, `UPPER_SNAKE`) | Pass | New names: `create_pen`, `dot_positions`, `draw_dots`, `main`; constants `GRID_ROWS`, `GRID_COLUMNS`, `DOT_SIZE`, `DOT_SPACING`; `Position` and the protocol `DotPen`; test classes `RecordingPen`, `MainRun`, `RecordingScreen`; helpers `_item_option`, `_drawn_dots`, `_other_lines`, `_write_stripes`. The `ruff` rule set `N` is on and clean |
+| 2 | Names state purpose in the domain's language; no unexplained abbreviations, no single-letter names outside tiny scopes | Pass | Names follow the PO terms of [BC-001] (dot, palette, painting, grid, colour). The only one-letter names are the coordinates `x` and `y`, in the tests, in scopes of a few lines. `RGB` appears in docstrings as the domain's own term |
+| 3 | Code is produced by the project's formatter and passes its linter with no unexplained suppressions | Pass | Both commands above are clean. The one suppression, `# type: ignore[no-untyped-call]` in `_item_option`, sits in a single helper with a comment saying that typeshed leaves `Canvas.itemcget` untyped; strict mypy would report it as unused if it stopped being needed |
+| 4 | Every function and method signature is type-annotated, including `-> None` | Pass | The syntax-tree scan shows every function in `src/` annotated, including the two `DotPen` methods; every test, fixture, helper and fake method in `tests/` is annotated; `ANN` rules and `mypy --strict` enforce it |
+| 5 | No bare `except:`, no swallowed exceptions; specific exceptions are raised and the cause is kept (`raise ... from`) | Pass | The code has no `try` or `except`. `draw_dots` raises `ValueError` with a message for an empty palette, and its docstring says so; `test_draw_dots_raises_when_the_palette_is_empty` proves it. A missing reference image raises `FileNotFoundError` out of `main`, unhandled and visible. No exception is re-raised, so no `raise ... from` is needed |
+| 6 | No mutable default arguments and no shadowed builtins | Pass | The only defaults are the integer `EXTRACTED_COLOUR_COUNT` and the function reference `random.choice`; the scan finds no mutable or call-valued default and no shadowed builtin (see the note on `__init__` above) |
+| 7 | Files, locks and connections are managed with context managers | Pass | `src/` opens nothing itself. The tests do hold a resource, the hidden Tk window; it is created and destroyed by the `tk_root` and `canvas` fixtures, the form pytest gives to context-managed set-up and tear-down |
+| 8 | Public modules, classes and functions have docstrings that say what, not how | Pass | Module, every public function and the `DotPen` protocol and its methods have docstrings; `ruff` rules `D` (pydocstyle, PEP 257) are on and clean. Observation, not a defect: the `...` after each docstring in `DotPen` is redundant and can be dropped |
+| 9 | Logging uses `logging`, not `print`; no secrets or personal data in log output | Pass | `src/` has no `print`, no logging and no secrets; the program reads `assets/20260524_132700.jpg` and nothing else. It does not read `.env` |
+| 10 | Classes and operations trace to the Design Class Diagram they implement; deviations are recorded | N-A | No Design Class Diagram exists: design artifacts are out of scope in [BC-001]. The only class-like element, the protocol `DotPen`, exists so that `draw_dots` can be tested without a window. The functions trace to the tasks of [MIL-002] instead: `create_pen` to task 1, `dot_positions` to task 2, `draw_dots` to task 3, `main` to tasks 4 and 5 |
+| 11 | Tests exist for new behaviour, are named for the behaviour, and do not depend on order or the network | Pass | 22 new tests named `test_<behaviour>_<condition>`, covering tasks 1 to 5 and the checks of task 6 (100 positions in 10 rows of 10, spacing 50, palette membership). Order independence was run, not assumed. The tests need a display for Tk (see the action item); they need no network. Review found one flaw during development, fixed before this record: starting Tk once per test failed now and then on Windows, so all tests share one hidden Tk window (`tk_root`, scope `session`) |
+| 12 | Type checker runs in strict mode without errors; `Any` is justified in a comment | Pass | `mypy` strict is clean and the code contains no `Any`. The `colorgram` override from [MIL-001] still has its comment in `pyproject.toml` |
+| 13 | Dependencies are declared and pinned in the project's dependency file, none unused | Pass | `pyproject.toml` is unchanged from the version merged with [MIL-001] and still pins `colorgram.py==1.2.0` and the dev tools `mypy==2.4.0`, `pillow==12.3.0`, `pytest==9.1.1` and `ruff==0.16.10`. The new code adds only standard-library imports (`random`, `turtle`, `tkinter`, `dataclasses`, `typing`) |
+
+## Overall Verdict
+
+Go — confirmed by S01 on 2026-10-08. All 12 applicable criteria of [QC-PY-001] pass,
+including the 3 optional ones (8, 12, 13); criterion 10 is N-A with the reason
+above. A `Go` here is Go criterion 6 of [MIL-002], the last open one. The
+code satisfies criteria 1 to 5 of [MIL-002] on the evidence above. Action items
+2 and 3 are decisions for S01 and do not block the verdict; only item 1 did, and it is closed.
+
+## Action Items
+
+| # | Action | Owner | Due |
+| --- | --- | --- | --- |
+| 1 | Confirm the verdict `Go` for the code of [MIL-002], or name the criterion you disagree with. **Closed 2026-10-08:** S01 confirmed `Go` | S01 | 2026-10-10 |
+| 2 | Decide whether the tests may need a display: they cannot run on a machine without one, such as a build server. Accept this for the project, or ask for a change. **Open:** a decision for S01 that does not block the verdict | S01 | 2026-10-10 |
+| 3 | Decide whether to raise the white threshold or reduce the colour count, because a few pale colours of the palette are faint on the white background (observation from the captured picture, not a criterion of [QC-PY-001]). Raising the threshold changes SC3 of [BC-001] and so needs a new row and a re-review of it; reducing the colour count does not. **Open:** a decision for S01 that does not block the verdict | S01 | 2026-10-10 |
+
+---
+
+[MIL-002]: ../../milestones/mil-002-painting.md
+[MIL-001]: ../../milestones/mil-001-palette.md
+[BC-001]: ../../business-case.md
+[QC-PY-001]: ../../../framework/qc/qc-programming-python.md
+[c3cfb64]: https://git.tirsystem.com/Tirsvad-Udemy-100-days-of-code/018-hirst-painting/commit/c3cfb6462e4b8b0a6fe37cf953a601703e352538

src/hirst_painting.py

@@ -1,13 +1,19 @@
 """Hirst-style spot painting drawn with turtle."""
 
-from collections.abc import Iterable
+import random
+import turtle
+from collections.abc import Callable, Iterable, Sequence
 from pathlib import Path
+from typing import Protocol
 
 import colorgram
 
 type Colour = tuple[int, int, int]
 """A colour as red, green and blue values from 0 to 255."""
 
+type Position = tuple[int, int]
+"""A point on the turtle screen as x and y, in screen units."""
+
 REFERENCE_IMAGE_PATH = (
     Path(__file__).resolve().parent.parent / "assets" / "20260524_132700.jpg"
 )
@@ -16,6 +22,14 @@ REFERENCE_IMAGE_PATH = (
 # has fewer, so the palette is never padded.
 EXTRACTED_COLOUR_COUNT = 30
 
+GRID_ROWS = 10
+GRID_COLUMNS = 10
+
+DOT_SIZE = 20
+
+# Distance between the centres of neighbouring dots, along rows and columns.
+DOT_SPACING = 50
+
 # A colour counts as a white shade when red, green and blue are all at or above
 # this value: such dots are invisible on the white background (SC3 of BC-001).
 WHITE_THRESHOLD = 240
@@ -41,3 +55,75 @@ def extract_palette(
     extracted = colorgram.extract(str(image_path), colour_count)
     colours = [(found.rgb.r, found.rgb.g, found.rgb.b) for found in extracted]
     return remove_white_shades(colours)
+
+
+class DotPen(Protocol):
+    """The two turtle operations that drawing dots needs."""
+
+    def goto(self, position: Position, /) -> None:
+        """Move to the position."""
+        ...
+
+    def dot(self, size: int, colour: Colour, /) -> None:
+        """Draw a dot of the size and colour at the current position."""
+        ...
+
+
+def create_pen(screen: turtle.TurtleScreen) -> turtle.RawTurtle:
+    """Return a hidden turtle with its pen up on a screen that takes RGB colours.
+
+    The pen is up and the turtle is hidden so that the painting shows no trail
+    and no cursor; the screen takes colours as 0 to 255 values, as `Colour` does.
+    """
+    screen.colormode(255)
+    pen = turtle.RawTurtle(screen)
+    pen.hideturtle()
+    pen.penup()
+    return pen
+
+
+def dot_positions() -> list[Position]:
+    """Return the centre of every dot, rows from the bottom up, left to right.
+
+    The grid is centred on the origin, the middle of the turtle window, so the
+    whole painting fits in a window of the default size.
+    """
+    left = -DOT_SPACING * (GRID_COLUMNS - 1) // 2
+    bottom = -DOT_SPACING * (GRID_ROWS - 1) // 2
+    return [
+        (left + column * DOT_SPACING, bottom + row * DOT_SPACING)
+        for row in range(GRID_ROWS)
+        for column in range(GRID_COLUMNS)
+    ]
+
+
+def draw_dots(
+    pen: DotPen,
+    palette: Sequence[Colour],
+    choose_colour: Callable[[Sequence[Colour]], Colour] = random.choice,
+) -> None:
+    """Draw a dot at every dot position, each in a colour chosen from the palette.
+
+    Raises ValueError when the palette has no colours.
+    """
+    if not palette:
+        raise ValueError("The palette has no colours to draw with.")
+    for position in dot_positions():
+        pen.goto(position)
+        pen.dot(DOT_SIZE, choose_colour(palette))
+
+
+def main() -> None:
+    """Draw the painting with the reference image's palette, then wait for a click."""
+    screen = turtle.Screen()
+    # Animating the 100 moves takes about 20 seconds on screen, against the 30
+    # allowed (SC6 of BC-001), so draw unseen and show the whole painting at once.
+    screen.tracer(0)
+    pen = create_pen(screen)
+    draw_dots(pen, extract_palette(REFERENCE_IMAGE_PATH))
+    screen.update()
+    screen.exitonclick()
+
+
+if __name__ == "__main__":
+    main()

tests/test_hirst_painting.py

@@ -1,15 +1,27 @@
 """Tests for the colour palette helpers of the Hirst painting program."""
 
+import tkinter
+import turtle
+from collections import Counter
+from collections.abc import Iterator, Sequence
+from dataclasses import dataclass, field
 from pathlib import Path
 
 import pytest
 from PIL import Image
 
+import hirst_painting
 from hirst_painting import (
     REFERENCE_IMAGE_PATH,
     Colour,
+    DotPen,
+    Position,
+    create_pen,
+    dot_positions,
+    draw_dots,
     extract_palette,
     is_white_shade,
+    main,
     remove_white_shades,
 )
 
@@ -118,3 +130,273 @@ def test_reference_image_palette_has_two_colours_and_no_white_shade() -> None:
 
     assert len(palette) >= 2
     assert not any(is_white_shade(colour) for colour in palette)
+
+
+@pytest.fixture(scope="session")
+def tk_root() -> Iterator[tkinter.Tk]:
+    """Create one hidden Tk window for the whole run.
+
+    Starting Tk again and again in one process fails now and then on Windows
+    (Tk cannot read its own library files), so every test shares this one.
+    """
+    root = tkinter.Tk()
+    root.withdraw()
+    yield root
+    root.destroy()
+
+
+@pytest.fixture
+def canvas(tk_root: tkinter.Tk) -> Iterator[tkinter.Canvas]:
+    """Yield a canvas on the hidden window, so no window appears."""
+    canvas = tkinter.Canvas(tk_root)
+    yield canvas
+    canvas.destroy()
+
+
+@pytest.fixture
+def screen(canvas: tkinter.Canvas) -> turtle.TurtleScreen:
+    """Return a turtle screen that draws on the hidden canvas."""
+    return turtle.TurtleScreen(canvas)
+
+
+def test_create_pen_hides_the_turtle(screen: turtle.TurtleScreen) -> None:
+    assert not create_pen(screen).isvisible()
+
+
+def test_create_pen_lifts_the_pen(screen: turtle.TurtleScreen) -> None:
+    assert not create_pen(screen).isdown()
+
+
+def test_create_pen_switches_the_screen_to_255_level_colours(
+    screen: turtle.TurtleScreen,
+) -> None:
+    create_pen(screen)
+
+    assert screen.colormode() == 255
+
+
+def test_create_pen_draws_on_the_given_screen(screen: turtle.TurtleScreen) -> None:
+    assert create_pen(screen).getscreen() is screen
+
+
+def test_dot_positions_returns_100_positions() -> None:
+    assert len(dot_positions()) == 100
+
+
+def test_dot_positions_are_all_different() -> None:
+    positions = dot_positions()
+
+    assert len(set(positions)) == len(positions)
+
+
+def test_dot_positions_have_10_heights_with_10_dots_each() -> None:
+    dots_per_height = Counter(y for _, y in dot_positions())
+
+    assert list(dots_per_height.values()) == [10] * 10
+
+
+def test_dot_positions_are_50_apart_along_each_row() -> None:
+    positions = dot_positions()
+
+    for start in range(0, 100, 10):
+        row = positions[start : start + 10]
+        assert [x for x, _ in row] == list(range(-225, 226, 50))
+        assert len({y for _, y in row}) == 1
+
+
+def test_dot_positions_are_50_apart_between_rows() -> None:
+    heights = sorted({y for _, y in dot_positions()})
+
+    assert heights == list(range(-225, 226, 50))
+
+
+def test_dot_positions_start_bottom_left_and_fill_rows_upward() -> None:
+    positions = dot_positions()
+
+    assert positions[0] == (-225, -225)
+    assert positions[1] == (-175, -225)
+    assert positions[10] == (-225, -175)
+    assert positions[-1] == (225, 225)
+
+
+def test_dot_positions_are_centred_on_the_origin() -> None:
+    positions = dot_positions()
+
+    assert min(x for x, _ in positions) == -max(x for x, _ in positions)
+    assert min(y for _, y in positions) == -max(y for _, y in positions)
+
+
+class RecordingPen:
+    """A pen that records the dots it was asked to draw and where."""
+
+    def __init__(self) -> None:
+        """Start at the origin with no dots drawn."""
+        self.position: Position = (0, 0)
+        self.dots: list[tuple[Position, int, Colour]] = []
+
+    def goto(self, position: Position, /) -> None:
+        """Remember the position the next dot is drawn at."""
+        self.position = position
+
+    def dot(self, size: int, colour: Colour, /) -> None:
+        """Record a dot at the current position."""
+        self.dots.append((self.position, size, colour))
+
+
+PALETTE: list[Colour] = [(200, 30, 40), (30, 90, 160), (20, 120, 60)]
+
+
+def test_draw_dots_draws_a_dot_at_every_position_in_order() -> None:
+    pen = RecordingPen()
+
+    draw_dots(pen, PALETTE)
+
+    assert [position for position, _, _ in pen.dots] == dot_positions()
+
+
+def test_draw_dots_draws_dots_of_size_20() -> None:
+    pen = RecordingPen()
+
+    draw_dots(pen, PALETTE)
+
+    assert {size for _, size, _ in pen.dots} == {20}
+
+
+def test_draw_dots_colours_every_dot_from_the_palette() -> None:
+    pen = RecordingPen()
+
+    draw_dots(pen, PALETTE)
+
+    assert {colour for _, _, colour in pen.dots} <= set(PALETTE)
+
+
+def test_draw_dots_asks_the_chooser_for_the_colour_of_each_dot() -> None:
+    pen = RecordingPen()
+    asked: list[Sequence[Colour]] = []
+
+    def choose_last(colours: Sequence[Colour]) -> Colour:
+        asked.append(colours)
+        return colours[-1]
+
+    draw_dots(pen, PALETTE, choose_colour=choose_last)
+
+    assert len(asked) == 100
+    assert {colour for _, _, colour in pen.dots} == {(20, 120, 60)}
+
+
+def test_draw_dots_raises_when_the_palette_is_empty() -> None:
+    with pytest.raises(ValueError, match="no colours"):
+        draw_dots(RecordingPen(), [])
+
+
+def _item_option(canvas: tkinter.Canvas, item: int, option: str) -> str:
+    """Return one option of a canvas item, such as its width or fill colour."""
+    # typeshed leaves Canvas.itemcget untyped, which strict mypy refuses to call.
+    return str(canvas.itemcget(item, option))  # type: ignore[no-untyped-call]
+
+
+def _drawn_dots(canvas: tkinter.Canvas) -> list[tuple[Position, str]]:
+    """Return the position and fill colour of every dot on the canvas.
+
+    Turtle draws a dot as a round-capped line whose width is the dot size.
+    """
+    dots: list[tuple[Position, str]] = []
+    for item in canvas.find_all():
+        is_dot = (
+            str(canvas.type(item)) == "line"
+            and _item_option(canvas, item, "width") == "20.0"
+        )
+        if is_dot:
+            x, y = canvas.coords(item)[:2]
+            dots.append(((round(x), -round(y)), _item_option(canvas, item, "fill")))
+    return dots
+
+
+def _other_lines(canvas: tkinter.Canvas) -> int:
+    """Count the line items that are not dots, such as a trail behind the pen."""
+    lines = [item for item in canvas.find_all() if str(canvas.type(item)) == "line"]
+    return len(lines) - len(_drawn_dots(canvas))
+
+
+def test_draw_dots_draws_100_dots_at_the_dot_positions_on_a_real_turtle(
+    screen: turtle.TurtleScreen, canvas: tkinter.Canvas
+) -> None:
+    screen.tracer(0)
+    pen = create_pen(screen)
+
+    draw_dots(pen, PALETTE)
+
+    positions = [position for position, _ in _drawn_dots(canvas)]
+    assert sorted(positions) == sorted(dot_positions())
+
+
+def test_draw_dots_colours_the_dots_of_a_real_turtle_from_the_palette(
+    screen: turtle.TurtleScreen, canvas: tkinter.Canvas
+) -> None:
+    screen.tracer(0)
+    pen = create_pen(screen)
+
+    draw_dots(pen, PALETTE)
+
+    palette_fills = {f"#{red:02x}{green:02x}{blue:02x}" for red, green, blue in PALETTE}
+    assert {fill for _, fill in _drawn_dots(canvas)} <= palette_fills
+
+
+def test_draw_dots_draws_no_trail_between_dots(
+    screen: turtle.TurtleScreen, canvas: tkinter.Canvas
+) -> None:
+    screen.tracer(0)
+    pen = create_pen(screen)
+    other_lines_before = _other_lines(canvas)
+
+    draw_dots(pen, PALETTE)
+
+    assert _other_lines(canvas) == other_lines_before
+
+
+@dataclass
+class MainRun:
+    """What happened during one run of main() on a fake screen."""
+
+    events: list[str] = field(default_factory=list)
+    palettes: list[Sequence[Colour]] = field(default_factory=list)
+    animation_while_drawing: list[int] = field(default_factory=list)
+
+
+@pytest.fixture
+def main_run(monkeypatch: pytest.MonkeyPatch, canvas: tkinter.Canvas) -> MainRun:
+    """Run main() once with drawing and the click wait replaced by recorders."""
+    run = MainRun()
+
+    class RecordingScreen(turtle.TurtleScreen):
+        def update(self) -> None:
+            run.events.append("update")
+
+        def exitonclick(self) -> None:
+            run.events.append("exitonclick")
+
+    screen = RecordingScreen(canvas)
+
+    def record_draw(pen: DotPen, palette: Sequence[Colour]) -> None:
+        run.events.append("draw_dots")
+        run.palettes.append(palette)
+        run.animation_while_drawing.append(screen.tracer())
+
+    monkeypatch.setattr(turtle, "Screen", lambda: screen)
+    monkeypatch.setattr(hirst_painting, "draw_dots", record_draw)
+    main()
+    return run
+
+
+def test_main_draws_with_the_palette_of_the_reference_image(
+    main_run: MainRun,
+) -> None:
+    assert main_run.palettes == [extract_palette(REFERENCE_IMAGE_PATH)]
+
+
+def test_main_turns_the_animation_off_while_drawing(main_run: MainRun) -> None:
+    assert main_run.animation_while_drawing == [0]
+
+
+def test_main_shows_the_painting_then_waits_for_a_click(main_run: MainRun) -> None:
+    assert main_run.events == ["draw_dots", "update", "exitonclick"]