From 814ffe0b2ac944bf755da2f47e3ef205b474230a Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Thu, 4 Jun 2026 16:35:21 +0100 Subject: [PATCH 1/7] refactor: simplify catalog loading to prioritize development repo files and update developer documentation --- doc/developer.catalog.rst | 9 ++++----- src/pygambit/catalog.py | 14 +++++++------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/doc/developer.catalog.rst b/doc/developer.catalog.rst index bbed7b3e4..fecc350f6 100644 --- a/doc/developer.catalog.rst +++ b/doc/developer.catalog.rst @@ -69,8 +69,7 @@ Currently supported representations are: 3. **Update the build files:** - Ensure you have installed the package in editable mode to automatically pick up the new game file(s) in the ``pygambit.catalog`` module without reinstalling each time. - Then use the ``update.py`` script to update Gambit's documentation & build files, as well as generating images for the new game(s). + Use the ``update.py`` script to update Gambit's documentation & build files, as well as generating images for the new game(s). If you want to customise the visualisation parameters for your game(s), edit ``build_support/catalog/draw_tree_settings.yaml``. Add an entry under ``overrides`` keyed by your game's exact slug, or by a shared prefix (e.g. the author-year folder name) to apply settings to all games from that source. More specific entries (longer keys) take precedence over shorter ones. @@ -82,12 +81,12 @@ Currently supported representations are: .. note:: - You can use the ``--regenerate-images`` flag when building the docs locally for a second time to force any changes to be picked up. + - The ``pygambit.catalog`` module reads games directly from the repo's ``catalog/`` directory when working in a development checkout. + - You can use the ``--regenerate-images`` flag when building the docs locally for a second time to force any changes to be picked up. .. warning:: - - If haven't done an editable install of ``pygambit`` in your python environment, you'll need to re-install it before running the update script to include new games in the catalog module. - - Running the script with the ``--build`` flag updates ``build_support/catalog/catalog.am``, which is included in ``Makefile.am``. If you moved games that were previously in ``contrib/games`` you'll need to also manually remove those files from ``EXTRA_DIST`` in ``Makefile.am``. + Running the script with the ``--build`` flag updates ``build_support/catalog/catalog.am``, which is included in ``Makefile.am``. If you moved games that were previously in ``contrib/games`` you'll need to also manually remove those files from ``EXTRA_DIST`` in ``Makefile.am``. 4. **[Optional] Test your updates to the documentation locally:** diff --git a/src/pygambit/catalog.py b/src/pygambit/catalog.py index 961c2a532..6c3010b05 100644 --- a/src/pygambit/catalog.py +++ b/src/pygambit/catalog.py @@ -6,13 +6,13 @@ import pygambit as gbt -# Use the full string path to where the catalog data are placed in the package -_CATALOG_RESOURCE = files("pygambit") / "catalog_data" -# This ensures that catalog files are included in editable installs too -if not _CATALOG_RESOURCE.is_dir(): - _repo_catalog = Path(__file__).parent.parent.parent / "catalog" - if _repo_catalog.is_dir(): - _CATALOG_RESOURCE = _repo_catalog +# Prefer the repo's live catalog/ directory when working in a development checkout. +# This ensures changes to catalog/ are picked up immediately without reinstalling, +# and avoids stale files in catalog_data/ left over from previous installs. +# Fall back to the installed catalog_data/ only for deployed (non-development) packages, +# where catalog/ does not exist relative to the installed source file. +_repo_catalog = Path(__file__).parent.parent.parent / "catalog" +_CATALOG_RESOURCE = _repo_catalog if _repo_catalog.is_dir() else files("pygambit") / "catalog_data" READERS = { ".nfg": gbt.read_nfg, From 46b623a475a75914b73cfaefa5ccc624ace7e0b5 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Thu, 4 Jun 2026 16:39:12 +0100 Subject: [PATCH 2/7] remove editable install suggestion --- doc/developer.catalog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer.catalog.rst b/doc/developer.catalog.rst index fecc350f6..ffae54e69 100644 --- a/doc/developer.catalog.rst +++ b/doc/developer.catalog.rst @@ -21,7 +21,7 @@ Currently supported representations are: .. code-block:: bash - pip install -e ".[doc]" + pip install ".[doc]" 1. **Create or edit a game file:** From e5da4027e679225d5dda2ab15eb4dff784244410 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Thu, 4 Jun 2026 16:42:08 +0100 Subject: [PATCH 3/7] docs: simplify local development instructions for updating the catalog --- doc/developer.catalog.rst | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/doc/developer.catalog.rst b/doc/developer.catalog.rst index ffae54e69..1867f414e 100644 --- a/doc/developer.catalog.rst +++ b/doc/developer.catalog.rst @@ -16,12 +16,7 @@ Currently supported representations are: .. important:: When updating the catalog, changes can be viewed by inspecting the documentation build generated by a pull request. - You can also test changes locally. It can be helpful when doing so to perform an editable install of `pygambit` available in your Python environment. - Note that for general edits to the codebase non-editable :ref:`developer install ` is better to pick up changes in the C++ code as well as Python. - - .. code-block:: bash - - pip install ".[doc]" + You can also test changes locally. To do this you should perform a :ref:`development install ` of `pygambit` in your python environment. 1. **Create or edit a game file:** From 9abaf2570599e3bf0f430683eae2e47517ebe6d4 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Fri, 5 Jun 2026 09:43:45 +0100 Subject: [PATCH 4/7] feat: add clean_stale_img_files utility and integrate into catalog update process --- build_support/catalog/test_update.py | 106 +++++++++++++++++++++++++++ build_support/catalog/update.py | 57 ++++++++++++++ 2 files changed, 163 insertions(+) diff --git a/build_support/catalog/test_update.py b/build_support/catalog/test_update.py index 70589e664..d1a5e6de6 100644 --- a/build_support/catalog/test_update.py +++ b/build_support/catalog/test_update.py @@ -581,6 +581,112 @@ def test_per_variant_images_not_regenerated_when_all_exist(self, tmp_path, monke assert calls == [] +# --------------------------------------------------------------------------- +# Tests for clean_stale_img_files +# --------------------------------------------------------------------------- + + +@pytest.mark.catalog_update +class TestCleanStaleImgFiles: + """Tests for ``clean_stale_img_files(catalog_dir)``. + + All tests construct a temporary catalog directory with hand-crafted game + files and img/ contents so they are completely isolated from the real + catalog on disk. + """ + + def _make_catalog(self, tmp_path): + """Return a minimal catalog dir with an img/ subdirectory.""" + catalog_dir = tmp_path / "catalog" + (catalog_dir / "img").mkdir(parents=True) + (catalog_dir / "img" / ".gitkeep").touch() + return catalog_dir + + def test_stale_file_is_removed(self, tmp_path): + """A file in img/ with no matching game file is deleted.""" + catalog_dir = self._make_catalog(tmp_path) + stale = catalog_dir / "img" / "oldgame.png" + stale.touch() + update.clean_stale_img_files(catalog_dir) + assert not stale.exists() + + def test_gitkeep_is_preserved(self, tmp_path): + """The .gitkeep sentinel is never removed, even when nothing else is kept.""" + catalog_dir = self._make_catalog(tmp_path) + (catalog_dir / "img" / "oldgame.png").touch() + update.clean_stale_img_files(catalog_dir) + assert (catalog_dir / "img" / ".gitkeep").exists() + + def test_expected_efg_files_are_kept(self, tmp_path): + """Image files that correspond to a current EFG game are not removed.""" + catalog_dir = self._make_catalog(tmp_path) + (catalog_dir / "src").mkdir() + (catalog_dir / "src" / "game.efg").touch() + for ext in ["ef", "tex", "png", "pdf", "svg"]: + (catalog_dir / "img" / f"src/game.{ext}").parent.mkdir(parents=True, exist_ok=True) + (catalog_dir / "img" / f"src/game.{ext}").touch() + update.clean_stale_img_files(catalog_dir) + for ext in ["ef", "tex", "png", "pdf", "svg"]: + assert (catalog_dir / "img" / f"src/game.{ext}").exists() + + def test_expected_nfg_files_are_kept(self, tmp_path): + """Image files for an NFG game (no .ef) are not removed.""" + catalog_dir = self._make_catalog(tmp_path) + (catalog_dir / "src").mkdir() + (catalog_dir / "src" / "matrix.nfg").touch() + (catalog_dir / "img" / "src").mkdir() + for ext in ["tex", "png", "pdf", "svg"]: + (catalog_dir / "img" / f"src/matrix.{ext}").touch() + update.clean_stale_img_files(catalog_dir) + for ext in ["tex", "png", "pdf", "svg"]: + assert (catalog_dir / "img" / f"src/matrix.{ext}").exists() + + def test_empty_directory_is_removed(self, tmp_path): + """A subdirectory of img/ that becomes empty after cleanup is also removed.""" + catalog_dir = self._make_catalog(tmp_path) + stale_dir = catalog_dir / "img" / "oldgroup" + stale_dir.mkdir() + (stale_dir / "oldgame.png").touch() + update.clean_stale_img_files(catalog_dir) + assert not stale_dir.exists() + + def test_nonempty_directory_is_kept(self, tmp_path): + """A subdirectory still containing expected files is not removed.""" + catalog_dir = self._make_catalog(tmp_path) + (catalog_dir / "src").mkdir() + (catalog_dir / "src" / "game.efg").touch() + (catalog_dir / "img" / "src").mkdir() + for ext in ["ef", "tex", "png", "pdf", "svg"]: + (catalog_dir / "img" / f"src/game.{ext}").touch() + update.clean_stale_img_files(catalog_dir) + assert (catalog_dir / "img" / "src").is_dir() + + def test_multi_variant_images_kept(self, tmp_path): + """Variant image files (e.g. {slug}__wide.*) are kept when the variant .ef exists.""" + catalog_dir = self._make_catalog(tmp_path) + game_dir = catalog_dir / "src" + game_dir.mkdir() + (game_dir / "game.efg").touch() + (game_dir / "game.ef").touch() + (game_dir / "game__wide.ef").touch() # triggers multi-variant + (catalog_dir / "img" / "src").mkdir() + for vkey in ["src/game", "src/game__wide"]: + for ext in ["ef", "tex", "png", "pdf", "svg"]: + p = catalog_dir / "img" / f"{vkey}.{ext}" + p.parent.mkdir(parents=True, exist_ok=True) + p.touch() + update.clean_stale_img_files(catalog_dir) + for vkey in ["src/game", "src/game__wide"]: + for ext in ["ef", "tex", "png", "pdf", "svg"]: + assert (catalog_dir / "img" / f"{vkey}.{ext}").exists() + + def test_noop_when_img_absent(self, tmp_path): + """If catalog/img/ does not exist, the function returns without error.""" + catalog_dir = tmp_path / "catalog" + catalog_dir.mkdir() + update.clean_stale_img_files(catalog_dir) # must not raise + + # --------------------------------------------------------------------------- # Tests for update_makefile # --------------------------------------------------------------------------- diff --git a/build_support/catalog/update.py b/build_support/catalog/update.py index 96c52f042..6173b5a43 100644 --- a/build_support/catalog/update.py +++ b/build_support/catalog/update.py @@ -253,6 +253,61 @@ def generate_rst_table( f.write(" \n") +def clean_stale_img_files(catalog_dir: Path | None = None) -> None: + """Remove files and directories from catalog/img/ with no corresponding current game. + + Computes the set of expected image files from the game files currently in + *catalog_dir* (all .efg and .nfg files, respecting multi-variant .ef conventions), + then removes anything in catalog/img/ that is not in that set. Preserves + .gitkeep. Empty directories left behind by file removal are also removed. + + This is useful after switching branches that reorganise or remove catalog games, + to prevent stale images from persisting alongside the updated catalog. + """ + catalog_dir = catalog_dir or CATALOG_DIR + img_dir = catalog_dir / "img" + if not img_dir.is_dir(): + return + + # Build the set of expected image file paths from the current catalog contents. + expected_files: set[Path] = set() + game_files = [ + p + for p in list(catalog_dir.rglob("*.efg")) + list(catalog_dir.rglob("*.nfg")) + if img_dir not in p.parents # exclude generated copies already inside img/ + ] + for game_file in game_files: + slug = game_file.relative_to(catalog_dir).with_suffix("").as_posix() + fmt = game_file.suffix.lstrip(".") # "efg" or "nfg" + ef_variants = catalog_ef_file_variants(slug, catalog_dir) if fmt == "efg" else None + if ef_variants: + for variant in ef_variants: + for ext in ["ef", "tex", "png", "pdf", "svg"]: + expected_files.add(img_dir / f"{variant['variant_key']}.{ext}") + else: + exts = (["ef"] if fmt == "efg" else []) + ["tex", "png", "pdf", "svg"] + for ext in exts: + expected_files.add(img_dir / f"{slug}.{ext}") + + # Remove unexpected files (preserving .gitkeep). + removed = 0 + for path in img_dir.rglob("*"): + if path.is_file() and path.name != ".gitkeep" and path not in expected_files: + path.unlink() + removed += 1 + + # Remove empty directories, deepest first. + for path in sorted(img_dir.rglob("*"), key=lambda p: len(p.parts), reverse=True): + if path.is_dir() and not any(path.iterdir()): + path.rmdir() + removed += 1 + + if removed: + print(f"Removed {removed} stale item(s) from {img_dir}") + else: + print(f"No stale files in {img_dir}") + + def update_makefile( catalog_dir: Path | None = None, am_path: Path | None = None, @@ -330,6 +385,8 @@ def update_makefile( ) args = parser.parse_args() + # Remove img/ files for games that no longer exist in the catalog. + clean_stale_img_files() # Create RST list-table used by doc/catalog.rst df = gbt.catalog.games(include_descriptions=True) generate_rst_table(df, CATALOG_RST_TABLE, regenerate_images=args.regenerate_images) From d3c9730aa12bd97127be634e9215454b96f3d4d2 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Fri, 5 Jun 2026 10:11:50 +0100 Subject: [PATCH 5/7] docs: re-clarify editable install instructions for catalog development in developer guide --- doc/developer.catalog.rst | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/doc/developer.catalog.rst b/doc/developer.catalog.rst index 485ec771c..c4ff05af8 100644 --- a/doc/developer.catalog.rst +++ b/doc/developer.catalog.rst @@ -16,7 +16,12 @@ Currently supported representations are: .. important:: When updating the catalog, changes can be viewed by inspecting the documentation build generated by a pull request. - You can also test changes locally. To do this you should perform a :ref:`development install ` of `pygambit` in your python environment. + You can also test changes locally. It can be helpful when doing so to perform an editable install of `pygambit` available in your Python environment. + Note that for general edits to the codebase non-editable :ref:`developer install ` is better to pick up changes in the C++ code as well as Python. + + .. code-block:: bash + + pip install -e ".[doc]" 1. **Create or edit a game file:** @@ -86,7 +91,8 @@ Currently supported representations are: 3. **Update the build files:** - Use the ``update.py`` script to update Gambit's documentation & build files, as well as generating images for the new game(s). + Ensure you have installed the package in editable mode to automatically pick up the new game file(s) in the ``pygambit.catalog`` module without reinstalling each time. + Then use the ``update.py`` script to update Gambit's documentation & build files, as well as generating images for the new game(s). If you want to customise the visualisation parameters for your game(s), edit ``build_support/catalog/draw_tree_settings.yaml``. Add an entry under ``overrides`` keyed by your game's exact slug, or by a shared prefix (e.g. the author-year folder name) to apply settings to all games from that source. More specific entries (longer keys) take precedence over shorter ones. @@ -103,7 +109,8 @@ Currently supported representations are: .. warning:: - Running the script with the ``--build`` flag updates ``build_support/catalog/catalog.am``, which is included in ``Makefile.am``. If you moved games that were previously in ``contrib/games`` you'll need to also manually remove those files from ``EXTRA_DIST`` in ``Makefile.am``. + - If haven't done an editable install of ``pygambit`` in your python environment, you'll need to re-install it before running the update script to include new games in the catalog module. + - Running the script with the ``--build`` flag updates ``build_support/catalog/catalog.am``, which is included in ``Makefile.am``. If you moved games that were previously in ``contrib/games`` you'll need to also manually remove those files from ``EXTRA_DIST`` in ``Makefile.am``. 4. **[Optional] Test your updates to the documentation locally:** From dfa6e08af7a2c9326fc178bbd7bde52a194bd9e4 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Fri, 5 Jun 2026 10:20:47 +0100 Subject: [PATCH 6/7] correct doc bullet on editable install --- doc/developer.catalog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer.catalog.rst b/doc/developer.catalog.rst index c4ff05af8..6c9cda489 100644 --- a/doc/developer.catalog.rst +++ b/doc/developer.catalog.rst @@ -104,7 +104,7 @@ Currently supported representations are: .. note:: - - The ``pygambit.catalog`` module reads games directly from the repo's ``catalog/`` directory when working in a development checkout. + - The ``pygambit.catalog`` module reads games directly from the repo's ``catalog/`` directory when working with an editable install of ``pygambit``. - You can use the ``--regenerate-images`` flag when building the docs locally for a second time to force any changes to be picked up. .. warning:: From ec108f37310fa5523ea3b5903bf78f301a559735 Mon Sep 17 00:00:00 2001 From: Ed Chalstrey Date: Fri, 5 Jun 2026 11:25:01 +0100 Subject: [PATCH 7/7] Revert pointless change --- build_support/catalog/test_update.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build_support/catalog/test_update.py b/build_support/catalog/test_update.py index 22385b9c7..dc540ccba 100644 --- a/build_support/catalog/test_update.py +++ b/build_support/catalog/test_update.py @@ -695,7 +695,7 @@ def test_second_level_dropdown_is_not_open(self, tmp_path, monkeypatch): assert " .. dropdown:: My Source\n \n" in rst # Confirm :open: does not immediately follow the second-level dropdown src_idx = rst.index(" .. dropdown:: My Source") - assert ":open:" not in rst[src_idx : src_idx + 40] # noqa: E203 + assert ":open:" not in rst[src_idx:src_idx + 40] def test_game_dropdown_is_open(self, tmp_path, monkeypatch): """Individual game dropdowns carry ``:open:`` so game content is visible on expand."""