Skip to content

Cookbook checker: verify each recipe imports the names its snippets use #75

Description

@craigmcchesney

Summary

.dev/tools/check-cookbook-snippets.py puts one shared preamble in front of every # cookbook:partial snippet. The preamble imports every alias any recipe uses, so a snippet that uses a name its own recipe never imports still passes, and fails with a NameError when a reader runs the recipe. #74 hit this twice (dfc in query.md, QueryParams / PV / bc in ingestion.md).

Plan: plan/tickets/75/plan.md (lands with the implementation, in one PR).

Scope (after triage, 2026-10-02)

  1. A third checker pass (pure ast, no new dependency). For each recipe file, every name a partial snippet uses that the preamble imports must be bound somewhere in that file's own checked blocks (imports, assignments, defs, …). The import list is parsed from the preamble itself, so the preamble's fixtures (client, params, begin/end, t0/t1, carried-forward ids, acquire()) are out of scope automatically. One error per name per file, at its first use.
  2. Unskip the six "Imports used by the examples" blocks. Triage finding not in the draft: all six are # cookbook:skip, so the imports block itself is never checked. A misspelled import there (PvQuerry as PV) passes today. With the skip removed, mypy catches it and the current cookbook still passes. The unskipped blocks also supply the bindings for (1), with no new directive.
  3. conventions.md and connecting.md get imports. The draft's open question resolves to "real hits": both uses are in checked code blocks. conventions.md gets its own imports block (Q, SavePvMetadataRequestParams, to_timestamp), and connecting.md's "Sub-clients can be None" block gets an inline import (QueryParams, PV). No opt-out directive.
  4. Self-test alongside the existing canary: a synthetic recipe using dfc without importing it must fail, and the same recipe with the import must pass.
  5. Docs: the checker docstring, doc/cookbook/README.md "Verifying the examples", and one sentence in CLAUDE.md. No NEXT.md entry, since this is tooling only (same call as Release notes checker: add link rules and fix the main-pinned link in rel-1.16.0 (data-platform#98) #70).

A prototype of (1) reports exactly the two #74 misses against the pre-fix files, and nothing in today's six recipes beyond the conventions.md / connecting.md hits.

Changed from the draft

  • ruff --select F821 is not used. mypy already reports any name the preamble does not define, so the only gap is names the preamble does define. F821 would double-report those and need a fixture stub header.
  • The draft's prose hits need no handling, because the checker only sees fenced ```python blocks.

Out of scope

  • Values carried between snippets (dataset_id, provider_id, …). Those still need the recipe run as one script, with only its own imports.
Original AI-drafted description

Problem

.dev/tools/check-cookbook-snippets.py puts one shared preamble in front of every # cookbook:partial block. The preamble imports every alias any recipe uses (qc, ssc, dfb, dfc, bc, PV, CFG, DS, AQ, …) and defines client, params, begin/end, t0/t1, and so on. So a snippet that uses a name its own recipe never imports still passes. Run on its own, the same recipe fails with a NameError.

This has now been hit twice in one PR (#74):

  • ingestion.md's imports block lacked QueryParams, PV, and bc. Running the recipe as a script caught it.
  • query.md used dfc and never imported it. This one survived the continuous-script run, because that run appended query.md after ingestion.md, which does import dfc. Copilot's review caught it. Fixed in eedae6a.

The cookbook promises that each recipe's "Imports used by the examples" block covers its examples, and nothing currently checks that.

Proposal

Add a per-recipe import check to the checker, alongside the existing syntax and type checks:

  1. For each recipe, collect the names its own code blocks bind. That means the imports block plus any inline import / from … import, assignments, and defs.
  2. Flag any module alias or imported name a block uses that the recipe never binds. The full list comes from the preamble's own import statements, so it stays in sync with the preamble automatically.
  3. Leave the preamble's fixtures out of this check (client, params, begin/end, t0/t1, the carried-forward ids, acquire()). Those stand in for values a recipe sets up in earlier prose or snippets, which is the reason the preamble exists.

One way to do this is to concatenate a recipe's blocks into one file, with only its own imports, and run ruff --select F821 (undefined name) over it. Fixture names would come from a stub header. Done by hand for the #74 fix, this found 4 undefined names before the fix and 0 after. A pure-ast walk would also work and adds no dependency.

Include a self-test case, matching the existing self-test style: a known-bad recipe that uses an alias it never imports must fail. That way, if the rule stops matching, the checker fails loudly instead of quietly passing everything.

Notes

  • A one-off scan of today's recipes found no other real misses. The other hits were aliases mentioned in prose (Q.attributes(...) in a callout, dfb.serialized_column() in a bullet), so the check should look only at code blocks.
  • connecting.md and conventions.md use PV / CFG / Q without importing them. Check whether those uses are in code blocks, and if so whether they are meant to rely on another recipe's imports. If they are, the new rule needs an explicit, documented opt-out rather than a silent exemption.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentationenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions