diff --git a/docs/artifact-registry.md b/docs/artifact-registry.md index a373739..7fb97a1 100644 --- a/docs/artifact-registry.md +++ b/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 diff --git a/docs/sqa/reviews/rc-006-code-mil-002.md b/docs/sqa/reviews/rc-006-code-mil-002.md new file mode 100644 index 0000000..eb300a6 --- /dev/null +++ b/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)
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__`, 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 diff --git a/src/hirst_painting.py b/src/hirst_painting.py index 387ba57..ee26d9a 100644 --- a/src/hirst_painting.py +++ b/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() diff --git a/tests/test_hirst_painting.py b/tests/test_hirst_painting.py index f89fa64..dab827c 100644 --- a/tests/test_hirst_painting.py +++ b/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"]