From 497c40bd19bedd166a88e2590fedf0ef985d3ad1 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 13:45:53 +0100 Subject: [PATCH 1/6] implement: Build only after a clean validate (t35) --- CHANGELOG.md | 1 + in2lambda/draft/export.py | 192 +++++++++++++++++++++++++++ in2lambda/main.py | 47 +++++++ in2lambda/validation/pdf/__init__.py | 167 ++++++++++++++++------- tests/conftest.py | 8 +- tests/test_cli.py | 2 + tests/test_draft.py | 107 ++++++++++++++- tests/test_validation.py | 10 +- 8 files changed, 472 insertions(+), 62 deletions(-) create mode 100644 in2lambda/draft/export.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bc651b..224ac3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,4 +11,5 @@ - `in2lambda draft field replace FIELD OLD NEW` changes the wording inside a field that is already written, for the faults only an edit can fix - a brace the OCR dropped out of some maths, which no range of the source says correctly. OLD has to occur in the field exactly once, or the command is refused saying how many times it occurs; `--regex` reads it as a regular expression and NEW as what to replace it with. The field is left quoting the lines it was taken from, at the layer that wrote it, but recorded as edited and by whoever replaced the wording, so the change can be shown against the source. - `in2lambda spec run SPEC` runs a YAML file of selectors over the frozen source: it says which blocks are questions, parts and solutions, which to ignore, what to strip off the front of each one, and which of the four filters lays the solutions out. It fills in the draft's fields with the markdown of the lines each was taken from, records the spec's name and hash in the log so a replay runs the same file, and reports every block it made nothing of. Running an edited spec over a draft it has already filled in is refused, as freezing a document that has changed is: `in2lambda source add --start-over` begins the draft again. Reading a spec needs pyyaml, which the `convert` extra now installs alongside panflute. See [the spec page](https://lambda-feedback.github.io/in2lambda/spec.html) for the selectors and layouts. - `in2lambda validate` checks a draft over as a whole and writes what it finds into it as a `report`: source blocks in no field and not marked ignore, two fields taken from the same lines, gaps in the numbering of the questions or their parts, parts nothing answers, and fields holding nothing. Each finding names the field and the lines it is about, so it can be acted on without reading the draft. Finding something is not a failure and the command still exits 0; the report is replaced by the next run of the checks and dropped by the next command that changes the draft, since it describes the draft as it stood. +- `in2lambda build` writes the draft in this directory out as a Lambda Feedback set: one question per `qN.text` field, holding the parts written for it and the worked solutions, with the images those fields refer to under `media/`, as `in2lambda convert` writes a set. It is refused unless `in2lambda validate` has been run since the draft last changed - every command that changes one drops its report - and found nothing, and the refusal prints what the report says so it can be acted on without opening the draft. `in2lambda render` writes each question as a PDF instead, compiled as Lambda Feedback's own PDF generator compiles it, which needs pandoc and xelatex; it is gated on nothing, since looking at a draft is how what the checks found gets fixed. Both take `-o/--out`, as `convert` does. - The Python API is unchanged: `in2lambda.main.runner` and everything under `in2lambda.api` take the same arguments and return the same objects. diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py new file mode 100644 index 0000000..3b2d0ec --- /dev/null +++ b/in2lambda/draft/export.py @@ -0,0 +1,192 @@ +"""Turns a finished draft into the set it describes, to upload or to look at. + +A draft is a map of fields - ``q1.text``, ``q1.p2.text``, ``q1.solution`` - and an +export is a :class:`~in2lambda.api.set.Set` of questions holding parts. :func:`as_set` +is the one place that reads the one as the other, so both what is written out and what +is rendered for review come from the same reading of the draft. + +:func:`build` refuses a draft the checks have not looked at, or have something to say +about. There is no timestamp in that: every command that changes a draft takes its +report with it, so a draft holding one has been checked since it last changed, and +`in2lambda.source.frozen` refuses one whose source has moved on underneath it. +:func:`render` is gated on nothing, since looking at a draft is how what the checks +found gets fixed. +""" + +import re +import warnings +from pathlib import Path +from typing import Any + +from in2lambda.api.part import Part +from in2lambda.api.question import Question +from in2lambda.api.set import Set +from in2lambda.json_convert.json_convert import _question_stem, _question_title +from in2lambda.source import DRAFT, ConversionToolsMissing, SourceError, frozen +from in2lambda.validation import _IMAGE, pdf + +_QUESTION = re.compile(r"q(\d+)\.text") +"""A question's text, and the number that orders it.""" + +_PART = re.compile(r"q(\d+)\.p(\d+)\.text") +"""A part's text, and the question and part numbers that order it.""" + + +class NotValidated(SourceError): + """A draft is being exported that the checks have not passed, or not seen at all.""" + + +def as_set(draft: dict[str, Any], directory: str = ".") -> Set: + r"""The set a draft's fields describe, in question and part order. + + Args: + draft: A draft, as `in2lambda.source.frozen` reads one. + directory: Where the draft is, and so what the images it names are beside. + + Returns: + One question per ``qN.text``, holding one part per ``qN.pM.text`` with the + worked solution written for it. A question's own ``qN.solution`` answers every + part that has none of its own, or is the whole of a question written without + parts, which is the rule :meth:`~in2lambda.api.question.Question.add_solution` + applies. A block marked ignore is in no question: it is the source's, not the + set's. + + Examples: + >>> from in2lambda.draft.export import as_set + >>> fields = { + ... "q1.text": {"value": "Water flows through a pipe."}, + ... "q1.p1.text": {"value": "State the continuity equation."}, + ... "q1.solution": {"value": "$Q = \\pi d^2 v / 4$."}, + ... "b1.ignore": {"value": True}, + ... } + >>> as_set({"fields": fields}).questions + [Question(title='', parts=[Part(text='State the continuity equation.', worked_solution='$Q = \\pi d^2 v / 4$.', answer='', response_areas=[])], images=[], main_text='Water flows through a pipe.')] + """ + fields = draft["fields"] + question_set = Set() + for number in sorted( + int(found[1]) for key in fields if (found := _QUESTION.fullmatch(key)) + ): + question_set.add_question(main_text=fields[f"q{number}.text"]["value"]) + question = question_set.questions[-1] + for part in sorted( + int(found[2]) + for key in fields + if (found := _PART.fullmatch(key)) and int(found[1]) == number + ): + question.add_part_text(fields[f"q{number}.p{part}.text"]["value"]) + if (written := f"q{number}.p{part}.solution") in fields: + question.parts[-1].worked_solution = fields[written]["value"] + if (written := f"q{number}.solution") in fields: + # A sheet often writes one worked solution for a whole question, which + # answers each part it does not answer separately; a question with no parts + # at all is the one part that solution belongs to. + if not question.parts: + question.parts.append(Part()) + for part_of in question.parts: + if not part_of.worked_solution: + part_of.worked_solution = fields[written]["value"] + # As the export refers to them: beside the draft, since that is where a command + # naming a file names one. Both the export's media/ and the renderer work from + # the question's images rather than from the references in its markdown. + question.images += [ + str(Path(directory) / reference) + for _, markdown in _fields(question, number) + for reference in _IMAGE.findall(markdown) + ] + return question_set + + +def build(directory: str = ".", output_dir: str = "out") -> Path: + """Writes the draft in a directory out as a Lambda Feedback set, if it is clean. + + Args: + directory: Where the ``draft.json`` to export is. + output_dir: Where to write the set's folder and its zip. + + Returns: + The zip that was written, which is what Lambda Feedback imports. + + Raises: + NotValidated: the draft has not been checked since it last changed, or the + checks found something. Either way what would be uploaded is not what + anybody has looked at. + SourceError: the draft is missing, is not one of ours, or was written from + markdown that has changed since. + """ + draft, _ = frozen(directory) + if "report" not in draft: + raise NotValidated( + f"{DRAFT} has not been validated since it last changed, so what it would " + "export is what nothing has checked. Run in2lambda validate." + ) + if draft["report"]: + raise NotValidated( + "\n".join(finding["message"] for finding in draft["report"]) + + f"\n{DRAFT} is not exported while its report says this. Fix what it " + "names, or mark the blocks it is about as ignored, and run in2lambda " + "validate again." + ) + as_set(draft, directory).to_json(output_dir) + return Path(output_dir) / "set.zip" + + +def render(directory: str = ".", output_dir: str = "out") -> list[Path]: + """Writes each question of the draft in a directory as a PDF, for review. + + The questions are compiled as Lambda Feedback's own PDF generator compiles them, + under a heading naming each, so what comes out is what a student would be shown. + The checks are not run first: looking at a draft is how what they found gets fixed. + + Args: + directory: Where the ``draft.json`` to render is. + output_dir: Where to write the PDFs, named as the export names its questions. + + Returns: + The PDF written for each question, in question order. + + Raises: + ConversionToolsMissing: pandoc or xelatex is not installed. + CompileFailed: a question produced no PDF at all. + SourceError: the draft is missing, is not one of ours, or was written from + markdown that has changed since. + + Warns: + UserWarning: once per LaTeX error in a question that was rendered anyway. + """ + if missing := pdf.missing_tools(): + raise ConversionToolsMissing( + f"Rendering questions needs {' and '.join(missing)}." + ) + draft, _ = frozen(directory) + + written = [] + for index, question in enumerate(as_set(draft, directory).questions): + stem = _question_stem(index, _question_title(question, index)) + output = Path(output_dir) / f"{stem}.pdf" + # Headed with the question's number, so that a stack of these can be read + # through, and so that a question with nothing written in it is still a page. + heading = f"Question {index + 1}" + fields = [(heading, f"# {heading}")] + _fields(question, index + 1) + for problem in pdf.render(fields, question.images, output): + warnings.warn(str(problem), stacklevel=2) + written.append(output) + return written + + +def _fields(question: Question, number: int) -> list[tuple[str, str]]: + """Every markdown field of a question, each with where to report an error in it. + + Named as `in2lambda.validation` names them, save for the title, since a draft + writes none: the renderer marks the document with these, so what it reports back + reads the same as what the validator reports. + """ + where = f"Question {number}" + fields = [(f"{where}, main text", question.main_text)] + for index, part in enumerate(question.parts): + part_where = f"{where}, part ({chr(ord('a') + index)})" + fields += [ + (f"{part_where}, text", part.text), + (f"{part_where}, worked solution", part.worked_solution), + ] + return fields diff --git a/in2lambda/main.py b/in2lambda/main.py index 9badce1..f59219b 100644 --- a/in2lambda/main.py +++ b/in2lambda/main.py @@ -21,6 +21,7 @@ import rich_click as click import in2lambda.draft +import in2lambda.draft.export import in2lambda.draft.report import in2lambda.filters import in2lambda.source @@ -444,5 +445,51 @@ def validate() -> None: click.echo("Nothing to report.") +_out = click.option( + "--out", + "-o", + "output_dir", + default="./out", + show_default=True, + help="Directory to write the files to.", + type=click.Path(resolve_path=True), +) +"""Where what a command makes is written, as `convert` has always taken it.""" + + +@cli.command("build") +@_out +def build(output_dir: str) -> None: + """Writes the draft in this directory out as a Lambda Feedback set. + + Refused unless in2lambda validate has been run since the draft last changed and + found nothing, so that what is uploaded is what the checks have been over. + """ + with _message_not_traceback(): + written = in2lambda.draft.export.build(output_dir=output_dir) + click.echo(f"Wrote {written}") + + +@cli.command("render") +@_out +def render(output_dir: str) -> None: + """Writes each question of the draft in this directory as a PDF, for review. + + The questions are compiled as Lambda Feedback's PDF generator compiles them, which + needs pandoc and xelatex. What the checks have to say about the draft is not asked: + a draft is rendered to look at, including one there is something to fix in. + """ + with _message_not_traceback(): + # As `runner` does: a question xelatex complains about is still written out, and + # what it refused is a line to read rather than a traceback. + with warnings.catch_warnings(record=True) as refused: + warnings.simplefilter("always") + written = in2lambda.draft.export.render(output_dir=output_dir) + for warning in refused: + click.echo(f"Warning: {warning.message}") + for pdf in written: + click.echo(f"Wrote {pdf}") + + if __name__ == "__main__": cli() diff --git a/in2lambda/validation/pdf/__init__.py b/in2lambda/validation/pdf/__init__.py index dfdf6f2..10ccf5b 100644 --- a/in2lambda/validation/pdf/__init__.py +++ b/in2lambda/validation/pdf/__init__.py @@ -12,6 +12,9 @@ pandoc and xelatex are both optional, as they are everywhere else in in2lambda: without them this reports what to install rather than raising. + +The same pipeline writes the PDF itself - :func:`render` - since what a reviewer wants +to look at is the document the errors were traced out of. """ import re @@ -21,6 +24,7 @@ from pathlib import Path from in2lambda.api.problem import Problem +from in2lambda.source import SourceError _TEMPLATE = Path(__file__).with_name("template.latex") """The PDF generator's own pandoc template - see the README beside it.""" @@ -51,6 +55,15 @@ """Seconds for pandoc or xelatex. A set that takes longer is reported, not waited for.""" +class CompileFailed(SourceError): + """The pipeline produced nothing: pandoc refused the set, or xelatex wrote no PDF. + + Not a problem in one field, since there is no generated LaTeX to trace an error back + through, so it is raised rather than reported - as a `SourceError`, which is what + the command line turns into a message rather than a traceback. + """ + + def missing_tools() -> list[str]: """What is needed to compile a set but is not installed, each saying how to get it. @@ -86,6 +99,8 @@ def problems(fields: list[tuple[str, str]], images: list[str]) -> list[Problem]: try: return _compiled(fields, images) + except CompileFailed as failed: + return [Problem(_SET, str(failed))] except subprocess.TimeoutExpired as expired: # TeX can be made to loop forever, which is itself a fault in the set. return [ @@ -97,57 +112,111 @@ def problems(fields: list[tuple[str, str]], images: list[str]) -> list[Problem]: ] -def _compiled(fields: list[tuple[str, str]], images: list[str]) -> list[Problem]: - """The set run through pandoc and then xelatex in a directory of its own.""" +def render( + fields: list[tuple[str, str]], images: list[str], output: Path +) -> list[Problem]: + """Writes the PDF Lambda Feedback's generator would make of these fields. + + Args: + fields: Every markdown field to render, each with the location to report an + error in it against, as :func:`problems` takes them. + images: Every image path the fields refer to, as :func:`problems` takes them. + output: The PDF file to write. Its directory is made if it is not there, and a + file of that name is overwritten. + + Returns: + One :class:`~in2lambda.api.problem.Problem` per LaTeX error, as + :func:`problems` reports them. xelatex typesets what it can whatever it + refuses, so these say what to look at in the PDF rather than that there is none. + + Raises: + CompileFailed: pandoc refused the fields, or xelatex wrote no PDF at all. + """ with tempfile.TemporaryDirectory() as directory: work = Path(directory) - available = set() - for image in images: - if Path(image).is_file(): - shutil.copy(image, work / Path(image).name) - available.add(Path(image).name) - - run = subprocess.run( - [ - "pandoc", - "-f", - "markdown-implicit_figures", - "-t", - "latex", - "-s", - f"--template={_TEMPLATE}", - "-o", - "set.tex", - ], - input=_marked_document(fields, available), - capture_output=True, - text=True, - # Not the locale's encoding: a set holding any non-ASCII character would - # then fail to even be handed over under, say, LC_ALL=C. - encoding="utf-8", - cwd=work, - timeout=_TIMEOUT, - ) - if run.returncode: - return [Problem(_SET, f"pandoc cannot read the set: {run.stderr.strip()}")] - - latex = (work / "set.tex").read_text(encoding="utf-8") - run = subprocess.run( - [ - "xelatex", - "-interaction=nonstopmode", - "-file-line-error", - "-no-shell-escape", - "set.tex", - ], - stdin=subprocess.DEVNULL, - capture_output=True, - text=True, - encoding="utf-8", - cwd=work, - timeout=_TIMEOUT, - ) - return _reported(run.stdout, _locations(latex)) + latex, log = _compile(fields, images, work) + problems = _reported(log, _locations(latex)) + if not (compiled := work / "set.pdf").is_file(): + raise CompileFailed( + f"xelatex produced no PDF of {output.name}" + + ( + ": " + "; ".join(str(problem) for problem in problems) + if problems + else "." + ) + ) + output.parent.mkdir(parents=True, exist_ok=True) + shutil.copy(compiled, output) + return problems + + +def _compiled(fields: list[tuple[str, str]], images: list[str]) -> list[Problem]: + """The set's errors, compiled in a directory of its own and thrown away again.""" + with tempfile.TemporaryDirectory() as directory: + latex, log = _compile(fields, images, Path(directory)) + return _reported(log, _locations(latex)) + + +def _compile( + fields: list[tuple[str, str]], images: list[str], work: Path +) -> tuple[str, str]: + """The set run through pandoc and then xelatex in `work`. + + Returns: + The LaTeX pandoc generated, whose markers say which field each line came from, + and the xelatex log. Whatever xelatex managed to typeset is left in `work` as + ``set.pdf``. + + Raises: + CompileFailed: pandoc would not read the set, so there is no LaTeX to run. + """ + available = set() + for image in images: + if Path(image).is_file(): + shutil.copy(image, work / Path(image).name) + available.add(Path(image).name) + + run = subprocess.run( + [ + "pandoc", + "-f", + "markdown-implicit_figures", + "-t", + "latex", + "-s", + f"--template={_TEMPLATE}", + "-o", + "set.tex", + ], + input=_marked_document(fields, available), + capture_output=True, + text=True, + # Not the locale's encoding: a set holding any non-ASCII character would + # then fail to even be handed over under, say, LC_ALL=C. + encoding="utf-8", + cwd=work, + timeout=_TIMEOUT, + ) + if run.returncode: + raise CompileFailed(f"pandoc cannot read the set: {run.stderr.strip()}") + + latex = (work / "set.tex").read_text(encoding="utf-8") + run = subprocess.run( + [ + "xelatex", + "-interaction=nonstopmode", + "-file-line-error", + "-no-shell-escape", + "set.tex", + ], + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + encoding="utf-8", + cwd=work, + timeout=_TIMEOUT, + ) + return latex, run.stdout def _marked_document(fields: list[tuple[str, str]], available: set[str]) -> str: diff --git a/tests/conftest.py b/tests/conftest.py index 2812361..0ae8848 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,7 +8,13 @@ import pytest import in2lambda -from in2lambda.validation import _node +from in2lambda.validation import _node, pdf + +needs_compiler = pytest.mark.skipif( + bool(pdf.missing_tools()), + reason="compiling the set as the PDF generator does needs pandoc and xelatex", +) +"""The fixtures are reported with the PDF generator's toolchain installed; CI has it.""" EXPORTS_DIR = Path(__file__).parent / "fixtures" / "exports" """Real Lambda Feedback exports, one set per folder, exactly as the platform wrote them.""" diff --git a/tests/test_cli.py b/tests/test_cli.py index 000ed2a..fab0b43 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -86,8 +86,10 @@ def test_completing_the_old_form_offers_the_subcommand() -> None: ).get_completions(["./questions.tex"], "") assert [candidate.value for candidate in completions] == [ + "build", "convert", "draft", + "render", "source", "spec", "validate", diff --git a/tests/test_draft.py b/tests/test_draft.py index 60bb9fa..134cf23 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -2,24 +2,31 @@ Each folder in ``fixtures/drafts`` is a document, the commands to run against its draft and the fields they should write, so covering another command means adding a folder -rather than a test. The rest is what the command line does when a replay cannot be -trusted - a source that has moved on, a log naming a command nothing has, a draft edited -by hand - which is not something a fixture can say. +rather than a test. Each is also what `in2lambda build` and `in2lambda render` make of +it: the one with no ``report.json`` is exported, the rest are refused. The remainder is +what the command line does when a replay cannot be trusted - a source that has moved on, +a log naming a command nothing has, a draft edited by hand - which is not something a +fixture can say. """ import json +import re import shutil from pathlib import Path from typing import Any import pytest from click.testing import CliRunner -from conftest import DRAFTS, DRAFTS_DIR +from conftest import DRAFTS, DRAFTS_DIR, needs_compiler import in2lambda.draft import in2lambda.draft.report +from in2lambda.api.set import Set from in2lambda.main import cli +QUESTION = re.compile(r"q(\d+)\.text") +"""A question's text among a folder's fields, which is one question of the export.""" + MARK_IGNORE = DRAFTS_DIR / "mark_ignore" """The case the tests below happen to use; what they check holds for any of them.""" @@ -485,3 +492,95 @@ def test_the_halves_of_a_split_block_are_blocks_like_any_other( # In the margin against the first line of each half, which is where the ids are. assert "b5a 10" in result.output assert "b5b 12" in result.output + + +def test_build_refuses_a_draft_that_has_not_been_validated( + tmp_path: Path, monkeypatch +) -> None: + """Every command that changes a draft drops its report, so this is every draft.""" + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + shutil.copytree(TWO_QUESTIONS, tmp_path, dirs_exist_ok=True) + assert CliRunner().invoke(cli, ["source", "add", "source.md"]).exit_code == 0 + for entry in json.loads((TWO_QUESTIONS / "commands.json").read_text()): + in2lambda.draft.execute(entry) + + result = CliRunner().invoke(cli, ["build"]) + + assert result.exit_code != 0 + assert "in2lambda validate" in result.output + assert not (tmp_path / "out").exists() + + +@pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) +def test_build_follows_the_report(folder: Path, tmp_path: Path, monkeypatch) -> None: + """A draft is exported once the checks have been over it and found nothing.""" + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + _built(folder, tmp_path) + fields = json.loads((folder / "expected.json").read_text()) + + result = CliRunner().invoke(cli, ["build"]) + + if report := _reported(folder): + assert result.exit_code != 0, result.output + # Every finding, so that what is left to do can be read off the refusal itself. + for finding in report: + assert finding["message"] in result.output + assert not (tmp_path / "out").exists() + return + + assert result.exit_code == 0, result.output + exported = tmp_path / "out" / "set.zip" + assert exported.is_file() + + questions = Set.from_json(str(exported)).questions + assert len(questions) == len([key for key in fields if QUESTION.fullmatch(key)]) + for number, question in enumerate(questions, start=1): + assert question.main_text == fields[f"q{number}.text"]["value"] + for index, part in enumerate(question.parts, start=1): + assert part.text == fields[f"q{number}.p{index}.text"]["value"] + # A part's own solution, or the question's where it has none of its own. + solution = fields.get( + f"q{number}.p{index}.solution", fields.get(f"q{number}.solution") + ) + assert part.worked_solution == (solution["value"] if solution else "") + + +@needs_compiler +@pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) +def test_render_writes_one_pdf_per_question( + folder: Path, tmp_path: Path, monkeypatch +) -> None: + """Rendering is for looking at a draft, so a draft with a report renders too.""" + monkeypatch.chdir(tmp_path) + _built(folder, tmp_path) + fields = json.loads((folder / "expected.json").read_text()) + + result = CliRunner().invoke(cli, ["render"]) + + assert result.exit_code == 0, result.output + written = sorted((tmp_path / "out").glob("*.pdf")) + assert len(written) == len([key for key in fields if QUESTION.fullmatch(key)]) + assert all(pdf.stat().st_size for pdf in written) + + +def test_render_says_what_to_install_without_the_compiler( + tmp_path: Path, monkeypatch +) -> None: + """The PDF generator's toolchain is optional, as it is everywhere else here.""" + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + _built(TWO_QUESTIONS, tmp_path) + which = shutil.which + monkeypatch.setattr( + shutil, + "which", + lambda command: None if command == "xelatex" else which(command), + ) + + result = CliRunner().invoke(cli, ["render"]) + + assert result.exit_code != 0 + assert "texlive-xetex" in result.output + assert not (tmp_path / "out").exists() diff --git a/tests/test_validation.py b/tests/test_validation.py index f58ddfb..3333bbf 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -20,19 +20,13 @@ from pathlib import Path import pytest -from conftest import EXPORTS, PROBLEM_SETS, PROBLEMS_DIR +from conftest import EXPORTS, PROBLEM_SETS, PROBLEMS_DIR, needs_compiler from in2lambda.api.set import Set -from in2lambda.validation import MathDelimiterError, _node, pdf, validate +from in2lambda.validation import MathDelimiterError, _node, validate E = MathDelimiterError -needs_compiler = pytest.mark.skipif( - bool(pdf.missing_tools()), - reason="compiling the set as the PDF generator does needs pandoc and xelatex", -) -"""The fixtures are reported with the PDF generator's toolchain installed; CI has it.""" - VALID = [ "This is an inline math expression: $x = y$.", "This is an inline math expression: $x = y$", From 8d96f9d382b6a2404c9c5142423eba67bfa3f35e Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 13:59:52 +0100 Subject: [PATCH 2/6] implement: Build only after a clean validate (t35) --- CHANGELOG.md | 2 +- in2lambda/draft/export.py | 32 ++++++++--- tests/fixtures/drafts/README.md | 4 +- .../drafts/figure_in_a_question/commands.json | 32 +++++++++++ .../drafts/figure_in_a_question/expected.json | 50 ++++++++++++++++++ .../drafts/figure_in_a_question/figure.png | Bin 0 -> 79 bytes .../drafts/figure_in_a_question/source.md | 9 ++++ tests/test_draft.py | 35 ++++++++++++ 8 files changed, 156 insertions(+), 8 deletions(-) create mode 100644 tests/fixtures/drafts/figure_in_a_question/commands.json create mode 100644 tests/fixtures/drafts/figure_in_a_question/expected.json create mode 100644 tests/fixtures/drafts/figure_in_a_question/figure.png create mode 100644 tests/fixtures/drafts/figure_in_a_question/source.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 224ac3b..5fc06fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,5 +11,5 @@ - `in2lambda draft field replace FIELD OLD NEW` changes the wording inside a field that is already written, for the faults only an edit can fix - a brace the OCR dropped out of some maths, which no range of the source says correctly. OLD has to occur in the field exactly once, or the command is refused saying how many times it occurs; `--regex` reads it as a regular expression and NEW as what to replace it with. The field is left quoting the lines it was taken from, at the layer that wrote it, but recorded as edited and by whoever replaced the wording, so the change can be shown against the source. - `in2lambda spec run SPEC` runs a YAML file of selectors over the frozen source: it says which blocks are questions, parts and solutions, which to ignore, what to strip off the front of each one, and which of the four filters lays the solutions out. It fills in the draft's fields with the markdown of the lines each was taken from, records the spec's name and hash in the log so a replay runs the same file, and reports every block it made nothing of. Running an edited spec over a draft it has already filled in is refused, as freezing a document that has changed is: `in2lambda source add --start-over` begins the draft again. Reading a spec needs pyyaml, which the `convert` extra now installs alongside panflute. See [the spec page](https://lambda-feedback.github.io/in2lambda/spec.html) for the selectors and layouts. - `in2lambda validate` checks a draft over as a whole and writes what it finds into it as a `report`: source blocks in no field and not marked ignore, two fields taken from the same lines, gaps in the numbering of the questions or their parts, parts nothing answers, and fields holding nothing. Each finding names the field and the lines it is about, so it can be acted on without reading the draft. Finding something is not a failure and the command still exits 0; the report is replaced by the next run of the checks and dropped by the next command that changes the draft, since it describes the draft as it stood. -- `in2lambda build` writes the draft in this directory out as a Lambda Feedback set: one question per `qN.text` field, holding the parts written for it and the worked solutions, with the images those fields refer to under `media/`, as `in2lambda convert` writes a set. It is refused unless `in2lambda validate` has been run since the draft last changed - every command that changes one drops its report - and found nothing, and the refusal prints what the report says so it can be acted on without opening the draft. `in2lambda render` writes each question as a PDF instead, compiled as Lambda Feedback's own PDF generator compiles it, which needs pandoc and xelatex; it is gated on nothing, since looking at a draft is how what the checks found gets fixed. Both take `-o/--out`, as `convert` does. +- `in2lambda build` writes the draft in this directory out as a Lambda Feedback set: one question per `qN.text` field, holding the parts written for it and the worked solutions, with the images those fields refer to under `media/`, as `in2lambda convert` writes a set - a field naming an image that is not beside the draft is refused saying which file is missing, since the checks read the draft and not the folder it is in. It is refused unless `in2lambda validate` has been run since the draft last changed - every command that changes one drops its report - and found nothing, and the refusal prints what the report says so it can be acted on without opening the draft. `in2lambda render` writes each question as a PDF instead, compiled as Lambda Feedback's own PDF generator compiles it, which needs pandoc and xelatex; it is gated on nothing, since looking at a draft is how what the checks found gets fixed. Both take `-o/--out`, as `convert` does. - The Python API is unchanged: `in2lambda.main.runner` and everything under `in2lambda.api` take the same arguments and return the same objects. diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py index 3b2d0ec..2b93a2d 100644 --- a/in2lambda/draft/export.py +++ b/in2lambda/draft/export.py @@ -36,6 +36,15 @@ class NotValidated(SourceError): """A draft is being exported that the checks have not passed, or not seen at all.""" +class MissingImage(SourceError): + """A field refers to an image file that is not beside the draft. + + The checks do not look at files, so such a draft validates clean; it is refused + here rather than exported, since what would be uploaded is a question with a broken + figure in it. + """ + + def as_set(draft: dict[str, Any], directory: str = ".") -> Set: r"""The set a draft's fields describe, in question and part order. @@ -51,6 +60,9 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: applies. A block marked ignore is in no question: it is the source's, not the set's. + Raises: + MissingImage: a field refers to an image file that is not beside the draft. + Examples: >>> from in2lambda.draft.export import as_set >>> fields = { @@ -88,12 +100,18 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: part_of.worked_solution = fields[written]["value"] # As the export refers to them: beside the draft, since that is where a command # naming a file names one. Both the export's media/ and the renderer work from - # the question's images rather than from the references in its markdown. - question.images += [ - str(Path(directory) / reference) - for _, markdown in _fields(question, number) - for reference in _IMAGE.findall(markdown) - ] + # the question's images rather than from the references in its markdown, so a + # file that is not there would be copied into media/ from nowhere. + for where, markdown in _fields(question, number): + for reference in _IMAGE.findall(markdown): + image = Path(directory) / reference + if not image.is_file(): + raise MissingImage( + f"{where} refers to the image {reference}, and there is no " + f"file at {image}. Put the image there, or take the reference " + "out of the field with in2lambda draft field replace." + ) + question.images.append(str(image)) return question_set @@ -111,6 +129,7 @@ def build(directory: str = ".", output_dir: str = "out") -> Path: NotValidated: the draft has not been checked since it last changed, or the checks found something. Either way what would be uploaded is not what anybody has looked at. + MissingImage: a field refers to an image file that is not beside the draft. SourceError: the draft is missing, is not one of ours, or was written from markdown that has changed since. """ @@ -148,6 +167,7 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: Raises: ConversionToolsMissing: pandoc or xelatex is not installed. CompileFailed: a question produced no PDF at all. + MissingImage: a field refers to an image file that is not beside the draft. SourceError: the draft is missing, is not one of ours, or was written from markdown that has changed since. diff --git a/tests/fixtures/drafts/README.md b/tests/fixtures/drafts/README.md index 3efb2d0..a28683c 100644 --- a/tests/fixtures/drafts/README.md +++ b/tests/fixtures/drafts/README.md @@ -22,4 +22,6 @@ both fields end up edited while their ranges still name the lines they were quot backslash in what the second one writes is written rather than read as a replacement template. `part_without_solution` and `empty_field` are the smallest drafts the other two checks have anything to say about; an overlap and a gap in the numbering are not here, because no run of -commands can make one. +commands can make one. `figure_in_a_question` is a question whose text runs on into an image, so +the field quotes the reference and `figure.png` beside the source is what the export has to carry +into `media/` - the only folder here with a file the fields point at. diff --git a/tests/fixtures/drafts/figure_in_a_question/commands.json b/tests/fixtures/drafts/figure_in_a_question/commands.json new file mode 100644 index 0000000..270cdbd --- /dev/null +++ b/tests/fixtures/drafts/figure_in_a_question/commands.json @@ -0,0 +1,32 @@ +[ + { + "args": { + "block": "b1" + }, + "by": "tests", + "command": "mark ignore" + }, + { + "args": { + "text": "b2" + }, + "by": "tests", + "command": "question add" + }, + { + "args": { + "question": "q1", + "text": "b3" + }, + "by": "tests", + "command": "part add" + }, + { + "args": { + "question": "q1", + "text": "b4" + }, + "by": "tests", + "command": "question solution" + } +] diff --git a/tests/fixtures/drafts/figure_in_a_question/expected.json b/tests/fixtures/drafts/figure_in_a_question/expected.json new file mode 100644 index 0000000..bd01b4a --- /dev/null +++ b/tests/fixtures/drafts/figure_in_a_question/expected.json @@ -0,0 +1,50 @@ +{ + "b1.ignore": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 1, + 1 + ] + ], + "value": true + }, + "q1.p1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 7, + 7 + ] + ], + "value": "Find the load the large piston carries when the small one is pressed with force $f$." + }, + "q1.solution": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 9, + 9 + ] + ], + "value": "The pressure is the same in both, so the load is $F = f A / a$." + }, + "q1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 3, + 5 + ] + ], + "value": "A hydraulic scale has two pistons joined by oil, the small one of area $a$ and the\nlarge one of area $A$.\n![The two pistons](figure.png)" + } +} diff --git a/tests/fixtures/drafts/figure_in_a_question/figure.png b/tests/fixtures/drafts/figure_in_a_question/figure.png new file mode 100644 index 0000000000000000000000000000000000000000..bb0e9db5b8feee30d75b2594fe8503bad747d0c1 GIT binary patch literal 79 zcmeAS@N?(olHy`uVBq!ia0vp^0wB!61SBU+%rFB|VxBIJAr-fhEec;sdUhm+9A44( ba5W=?Tup=g0aivEpb7?0S3j3^P6 Path: """A folder's document, frozen in `tmp_path` with its commands applied and checked.""" @@ -546,6 +550,37 @@ def test_build_follows_the_report(folder: Path, tmp_path: Path, monkeypatch) -> ) assert part.worked_solution == (solution["value"] if solution else "") + # Every image a field refers to travels with the set under media/, which is the only + # place Lambda Feedback looks for one; the set's folder is asked rather than the + # loaded questions, since reading an export back attributes an image to a question + # by the platform's own naming of the file, which a draft's images do not follow. + assert {path.name for path in (tmp_path / "out" / "set" / "media").glob("*")} == { + Path(reference).name + for field in fields.values() + if isinstance(field["value"], str) + for reference in _IMAGE.findall(field["value"]) + } + + +def test_build_refuses_a_field_naming_an_image_that_is_not_there( + tmp_path: Path, monkeypatch +) -> None: + """The checks read the draft and not the folder, so a clean one can still say this. + + Exporting it anyway would upload a question whose figure is a broken image, since + the file the markdown names is what the export carries under media/. + """ + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + _built(FIGURE, tmp_path) + (tmp_path / "figure.png").unlink() + + result = CliRunner().invoke(cli, ["build"]) + + assert result.exit_code != 0 + assert "figure.png" in result.output + assert not (tmp_path / "out").exists() + @needs_compiler @pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) From ee74c573e856e50b61f285c12fc8f8e1f98f0a83 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 14:17:51 +0100 Subject: [PATCH 3/6] implement: Build only after a clean validate (t35) --- in2lambda/draft/export.py | 3 ++- in2lambda/validation/pdf/__init__.py | 33 ++++++++++++++++++---------- tests/test_draft.py | 27 ++++++++++++++++++++++- 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py index 2b93a2d..b09512c 100644 --- a/in2lambda/draft/export.py +++ b/in2lambda/draft/export.py @@ -166,7 +166,8 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: Raises: ConversionToolsMissing: pandoc or xelatex is not installed. - CompileFailed: a question produced no PDF at all. + CompileFailed: a question produced no PDF at all, or the compiler did not + finish with it. MissingImage: a field refers to an image file that is not beside the draft. SourceError: the draft is missing, is not one of ours, or was written from markdown that has changed since. diff --git a/in2lambda/validation/pdf/__init__.py b/in2lambda/validation/pdf/__init__.py index 10ccf5b..61d17cc 100644 --- a/in2lambda/validation/pdf/__init__.py +++ b/in2lambda/validation/pdf/__init__.py @@ -56,7 +56,8 @@ class CompileFailed(SourceError): - """The pipeline produced nothing: pandoc refused the set, or xelatex wrote no PDF. + """The pipeline produced nothing: pandoc refused the set, xelatex wrote no PDF, or + neither finished. Not a problem in one field, since there is no generated LaTeX to trace an error back through, so it is raised rather than reported - as a `SourceError`, which is what @@ -101,15 +102,6 @@ def problems(fields: list[tuple[str, str]], images: list[str]) -> list[Problem]: return _compiled(fields, images) except CompileFailed as failed: return [Problem(_SET, str(failed))] - except subprocess.TimeoutExpired as expired: - # TeX can be made to loop forever, which is itself a fault in the set. - return [ - Problem( - _SET, - f"the PDF generator cannot compile this: {expired.cmd[0]} did not" - f" finish within {_TIMEOUT} seconds", - ) - ] def render( @@ -130,7 +122,8 @@ def render( refuses, so these say what to look at in the PDF rather than that there is none. Raises: - CompileFailed: pandoc refused the fields, or xelatex wrote no PDF at all. + CompileFailed: pandoc refused the fields, neither tool finished, or xelatex + wrote no PDF at all. """ with tempfile.TemporaryDirectory() as directory: work = Path(directory) @@ -168,8 +161,24 @@ def _compile( ``set.pdf``. Raises: - CompileFailed: pandoc would not read the set, so there is no LaTeX to run. + CompileFailed: pandoc would not read the set, so there is no LaTeX to run, or + one of the two did not finish. A set can be written that makes TeX loop + forever, which is a fault in the set like any other: both callers want it + said rather than waited for, so it is said here once. """ + try: + return _run(fields, images, work) + except subprocess.TimeoutExpired as expired: + raise CompileFailed( + f"the PDF generator cannot compile this: {expired.cmd[0]} did not" + f" finish within {_TIMEOUT} seconds" + ) from None + + +def _run( + fields: list[tuple[str, str]], images: list[str], work: Path +) -> tuple[str, str]: + """The two commands themselves, whatever either of them does.""" available = set() for image in images: if Path(image).is_file(): diff --git a/tests/test_draft.py b/tests/test_draft.py index 92d5028..3093d2a 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -12,6 +12,7 @@ import json import re import shutil +import subprocess from pathlib import Path from typing import Any @@ -23,7 +24,7 @@ import in2lambda.draft.report from in2lambda.api.set import Set from in2lambda.main import cli -from in2lambda.validation import _IMAGE +from in2lambda.validation import _IMAGE, pdf QUESTION = re.compile(r"q(\d+)\.text") """A question's text among a folder's fields, which is one question of the export.""" @@ -619,3 +620,27 @@ def test_render_says_what_to_install_without_the_compiler( assert result.exit_code != 0 assert "texlive-xetex" in result.output assert not (tmp_path / "out").exists() + + +def test_render_says_which_tool_never_finished(tmp_path: Path, monkeypatch) -> None: + """A set can be written that makes TeX loop, and waiting is not what happens then. + + The toolchain is stood in for rather than run, so that this says what the command + does with a timeout wherever it is run, not only where a compiler is installed. + """ + + def never_finishes(command: list[str], **_: Any) -> None: + raise subprocess.TimeoutExpired(command, pdf._TIMEOUT) + + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + _built(TWO_QUESTIONS, tmp_path) + monkeypatch.setattr(shutil, "which", lambda command: f"/usr/bin/{command}") + monkeypatch.setattr(subprocess, "run", never_finishes) + + result = CliRunner().invoke(cli, ["render"]) + + assert result.exit_code != 0 + # The line a reader can act on, rather than the traceback out of subprocess. + assert "pandoc did not finish" in result.output + assert not isinstance(result.exception, subprocess.TimeoutExpired) From 57dc02f396e926760983d42f2033a3bcaeae5e25 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 14:34:16 +0100 Subject: [PATCH 4/6] implement: Build only after a clean validate (t35) --- CHANGELOG.md | 2 +- in2lambda/draft/export.py | 69 ++++++++------- tests/fixtures/drafts/README.md | 4 + .../commands.json | 18 ++++ .../expected.json | 86 +++++++++++++++++++ .../source.md | 13 +++ .../spec.yaml | 6 ++ tests/test_draft.py | 49 +++++++++++ 8 files changed, 217 insertions(+), 30 deletions(-) create mode 100644 tests/fixtures/drafts/question_solution_beside_part_solutions/commands.json create mode 100644 tests/fixtures/drafts/question_solution_beside_part_solutions/expected.json create mode 100644 tests/fixtures/drafts/question_solution_beside_part_solutions/source.md create mode 100644 tests/fixtures/drafts/question_solution_beside_part_solutions/spec.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fc06fe..b2a7883 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,5 +11,5 @@ - `in2lambda draft field replace FIELD OLD NEW` changes the wording inside a field that is already written, for the faults only an edit can fix - a brace the OCR dropped out of some maths, which no range of the source says correctly. OLD has to occur in the field exactly once, or the command is refused saying how many times it occurs; `--regex` reads it as a regular expression and NEW as what to replace it with. The field is left quoting the lines it was taken from, at the layer that wrote it, but recorded as edited and by whoever replaced the wording, so the change can be shown against the source. - `in2lambda spec run SPEC` runs a YAML file of selectors over the frozen source: it says which blocks are questions, parts and solutions, which to ignore, what to strip off the front of each one, and which of the four filters lays the solutions out. It fills in the draft's fields with the markdown of the lines each was taken from, records the spec's name and hash in the log so a replay runs the same file, and reports every block it made nothing of. Running an edited spec over a draft it has already filled in is refused, as freezing a document that has changed is: `in2lambda source add --start-over` begins the draft again. Reading a spec needs pyyaml, which the `convert` extra now installs alongside panflute. See [the spec page](https://lambda-feedback.github.io/in2lambda/spec.html) for the selectors and layouts. - `in2lambda validate` checks a draft over as a whole and writes what it finds into it as a `report`: source blocks in no field and not marked ignore, two fields taken from the same lines, gaps in the numbering of the questions or their parts, parts nothing answers, and fields holding nothing. Each finding names the field and the lines it is about, so it can be acted on without reading the draft. Finding something is not a failure and the command still exits 0; the report is replaced by the next run of the checks and dropped by the next command that changes the draft, since it describes the draft as it stood. -- `in2lambda build` writes the draft in this directory out as a Lambda Feedback set: one question per `qN.text` field, holding the parts written for it and the worked solutions, with the images those fields refer to under `media/`, as `in2lambda convert` writes a set - a field naming an image that is not beside the draft is refused saying which file is missing, since the checks read the draft and not the folder it is in. It is refused unless `in2lambda validate` has been run since the draft last changed - every command that changes one drops its report - and found nothing, and the refusal prints what the report says so it can be acted on without opening the draft. `in2lambda render` writes each question as a PDF instead, compiled as Lambda Feedback's own PDF generator compiles it, which needs pandoc and xelatex; it is gated on nothing, since looking at a draft is how what the checks found gets fixed. Both take `-o/--out`, as `convert` does. +- `in2lambda build` writes the draft in this directory out as a Lambda Feedback set: one question per `qN.text` field, holding the parts written for it and the worked solutions, with the images those fields refer to under `media/`, as `in2lambda convert` writes a set - a field naming an image that is not beside the draft is refused saying which file is missing, since the checks read the draft and not the folder it is in, and a question's own solution written beside a solution for every part it has becomes a part of its own holding just that solution, as `convert` pairs them up. It is refused unless `in2lambda validate` has been run since the draft last changed - every command that changes one drops its report - and found nothing, and the refusal prints what the report says so it can be acted on without opening the draft. `in2lambda render` writes each question as a PDF instead, compiled as Lambda Feedback's own PDF generator compiles it, which needs pandoc and xelatex; it is gated on nothing, since looking at a draft is how what the checks found gets fixed. Both take `-o/--out`, as `convert` does. - The Python API is unchanged: `in2lambda.main.runner` and everything under `in2lambda.api` take the same arguments and return the same objects. diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py index b09512c..1fd88af 100644 --- a/in2lambda/draft/export.py +++ b/in2lambda/draft/export.py @@ -55,13 +55,12 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: Returns: One question per ``qN.text``, holding one part per ``qN.pM.text`` with the worked solution written for it. A question's own ``qN.solution`` answers every - part that has none of its own, or is the whole of a question written without - parts, which is the rule :meth:`~in2lambda.api.question.Question.add_solution` - applies. A block marked ignore is in no question: it is the source's, not the - set's. - - Raises: - MissingImage: a field refers to an image file that is not beside the draft. + part that has none of its own; where every part has one already, or the + question was written without parts, it becomes a part of its own holding + nothing but that solution. That is the rule + :meth:`~in2lambda.api.question.Question.add_solution` applies, so a draft + exports as the same sheet converted by `in2lambda convert` does. A block marked + ignore is in no question: it is the source's, not the set's. Examples: >>> from in2lambda.draft.export import as_set @@ -91,27 +90,24 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: question.parts[-1].worked_solution = fields[written]["value"] if (written := f"q{number}.solution") in fields: # A sheet often writes one worked solution for a whole question, which - # answers each part it does not answer separately; a question with no parts - # at all is the one part that solution belongs to. - if not question.parts: - question.parts.append(Part()) - for part_of in question.parts: - if not part_of.worked_solution: - part_of.worked_solution = fields[written]["value"] + # answers each part it does not answer separately. Where nothing is left for + # it to answer it is a part of its own, as `add_solution` makes it one: a + # solution written beside a solution for every part is still the author's + # wording, and dropping it would export less than the draft holds. + if all(part_of.worked_solution for part_of in question.parts): + question.parts.append(Part(worked_solution=fields[written]["value"])) + else: + for part_of in question.parts: + if not part_of.worked_solution: + part_of.worked_solution = fields[written]["value"] # As the export refers to them: beside the draft, since that is where a command - # naming a file names one. Both the export's media/ and the renderer work from - # the question's images rather than from the references in its markdown, so a - # file that is not there would be copied into media/ from nowhere. - for where, markdown in _fields(question, number): - for reference in _IMAGE.findall(markdown): - image = Path(directory) / reference - if not image.is_file(): - raise MissingImage( - f"{where} refers to the image {reference}, and there is no " - f"file at {image}. Put the image there, or take the reference " - "out of the field with in2lambda draft field replace." - ) - question.images.append(str(image)) + # naming a file names one. Whether the file is there is `build`'s question, not + # asked here, so that a draft can be rendered while its figures are being found. + for _, markdown in _fields(question, number): + question.images += [ + str(Path(directory) / reference) + for reference in _IMAGE.findall(markdown) + ] return question_set @@ -146,7 +142,21 @@ def build(directory: str = ".", output_dir: str = "out") -> Path: "names, or mark the blocks it is about as ignored, and run in2lambda " "validate again." ) - as_set(draft, directory).to_json(output_dir) + exported = as_set(draft, directory) + # The export carries every image a field refers to into media/, which is the only + # place Lambda Feedback looks for one, so a file that is not there is not something + # to write the set without: `json_convert` would raise a bare FileNotFoundError over + # it. The checks read the draft and not the folder it is in, so a draft they found + # nothing in can still say this. + for number, question in enumerate(exported.questions, start=1): + for image in question.images: + if not Path(image).is_file(): + raise MissingImage( + f"Question {number} refers to an image, and there is no file at " + f"{image}. Put the image there, or take the reference out of the " + "field with in2lambda draft field replace." + ) + exported.to_json(output_dir) return Path(output_dir) / "set.zip" @@ -156,6 +166,8 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: The questions are compiled as Lambda Feedback's own PDF generator compiles them, under a heading naming each, so what comes out is what a student would be shown. The checks are not run first: looking at a draft is how what they found gets fixed. + Nor does a figure that is not beside the draft stop a question being looked at - + the compiler drops the reference and typesets the rest of it. Args: directory: Where the ``draft.json`` to render is. @@ -168,7 +180,6 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: ConversionToolsMissing: pandoc or xelatex is not installed. CompileFailed: a question produced no PDF at all, or the compiler did not finish with it. - MissingImage: a field refers to an image file that is not beside the draft. SourceError: the draft is missing, is not one of ours, or was written from markdown that has changed since. diff --git a/tests/fixtures/drafts/README.md b/tests/fixtures/drafts/README.md index a28683c..e3fb0a6 100644 --- a/tests/fixtures/drafts/README.md +++ b/tests/fixtures/drafts/README.md @@ -25,3 +25,7 @@ anything to say about; an overlap and a gap in the numbering are not here, becau commands can make one. `figure_in_a_question` is a question whose text runs on into an image, so the field quotes the reference and `figure.png` beside the source is what the export has to carry into `media/` - the only folder here with a file the fields point at. +`question_solution_beside_part_solutions` is the only one filled in by a spec rather than by +commands one at a time, and the only one whose question has two parts: both are answered by the +spec's own solutions, so the `question solution` after it answers nothing, and the export carries +it as a part of its own rather than dropping the wording. diff --git a/tests/fixtures/drafts/question_solution_beside_part_solutions/commands.json b/tests/fixtures/drafts/question_solution_beside_part_solutions/commands.json new file mode 100644 index 0000000..0ce4a0a --- /dev/null +++ b/tests/fixtures/drafts/question_solution_beside_part_solutions/commands.json @@ -0,0 +1,18 @@ +[ + { + "args": { + "hash": "sha256:de8814ff58a658669eadd41c48481a5082b2bfd4242d43ad751ec79a66640108", + "spec": "spec.yaml" + }, + "by": "tests", + "command": "spec run" + }, + { + "args": { + "question": "q1", + "text": "b7" + }, + "by": "tests", + "command": "question solution" + } +] diff --git a/tests/fixtures/drafts/question_solution_beside_part_solutions/expected.json b/tests/fixtures/drafts/question_solution_beside_part_solutions/expected.json new file mode 100644 index 0000000..d372085 --- /dev/null +++ b/tests/fixtures/drafts/question_solution_beside_part_solutions/expected.json @@ -0,0 +1,86 @@ +{ + "b1.ignore": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 1, + 1 + ] + ], + "value": true + }, + "q1.p1.solution": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 9, + 9 + ] + ], + "value": "Each support carries $W/2$, by symmetry." + }, + "q1.p1.text": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 5, + 5 + ] + ], + "value": "Find the reaction at each support." + }, + "q1.p2.solution": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 11, + 11 + ] + ], + "value": "The moment is largest under the load, where it is $WL/4$." + }, + "q1.p2.text": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 7, + 7 + ] + ], + "value": "Find the maximum bending moment." + }, + "q1.solution": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 13, + 13 + ] + ], + "value": "Both answers are in SI units." + }, + "q1.text": { + "by": "tests", + "edited": false, + "layer": 1, + "ranges": [ + [ + 3, + 3 + ] + ], + "value": "A beam of length $L$ rests on two supports and carries a point load $W$ at its centre." + } +} diff --git a/tests/fixtures/drafts/question_solution_beside_part_solutions/source.md b/tests/fixtures/drafts/question_solution_beside_part_solutions/source.md new file mode 100644 index 0000000..596e2d7 --- /dev/null +++ b/tests/fixtures/drafts/question_solution_beside_part_solutions/source.md @@ -0,0 +1,13 @@ +# Statics problem sheet + +Q1. A beam of length $L$ rests on two supports and carries a point load $W$ at its centre. + +(a) Find the reaction at each support. + +(b) Find the maximum bending moment. + +Solution: Each support carries $W/2$, by symmetry. + +Solution: The moment is largest under the load, where it is $WL/4$. + +Both answers are in SI units. diff --git a/tests/fixtures/drafts/question_solution_beside_part_solutions/spec.yaml b/tests/fixtures/drafts/question_solution_beside_part_solutions/spec.yaml new file mode 100644 index 0000000..8f455de --- /dev/null +++ b/tests/fixtures/drafts/question_solution_beside_part_solutions/spec.yaml @@ -0,0 +1,6 @@ +question: Para text~'^Q\d+\.' +part: ListItem +solution: Para text~'^Solution:' +strip: ['^Q\d+\. ', '^\([a-z]\) ', '^Solution: '] +ignore: Header +layout: PartPartSolSol diff --git a/tests/test_draft.py b/tests/test_draft.py index 3093d2a..1896886 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -29,6 +29,9 @@ QUESTION = re.compile(r"q(\d+)\.text") """A question's text among a folder's fields, which is one question of the export.""" +PART = re.compile(r"q(\d+)\.p(\d+)\.text") +"""A part's text, which is one part of the question it is numbered under.""" + MARK_IGNORE = DRAFTS_DIR / "mark_ignore" """The case the tests below happen to use; what they check holds for any of them.""" @@ -51,6 +54,22 @@ def _built(folder: Path, tmp_path: Path) -> Path: return tmp_path / "draft.json" +def _expected_parts(fields: dict[str, Any], number: int) -> int: + """How many parts a question's fields describe, counted from the fields themselves. + + One per ``qN.pM.text``, and one more where ``qN.solution`` is written beside a + solution for every part there is: nothing is left for it to answer, so it is a part + of its own, as `in2lambda convert` writes one. + """ + written = [ + int(found[2]) + for key in fields + if (found := PART.fullmatch(key)) and int(found[1]) == number + ] + answered = all(f"q{number}.p{part}.solution" in fields for part in written) + return len(written) + (answered and f"q{number}.solution" in fields) + + def _reported(folder: Path) -> list[dict[str, Any]]: """What the checks should find in a folder's draft; nothing, where it says none.""" report = folder / "report.json" @@ -543,7 +562,16 @@ def test_build_follows_the_report(folder: Path, tmp_path: Path, monkeypatch) -> assert len(questions) == len([key for key in fields if QUESTION.fullmatch(key)]) for number, question in enumerate(questions, start=1): assert question.main_text == fields[f"q{number}.text"]["value"] + # Counted from the fields rather than read off the question, since a loop over + # parts that were dropped runs no assertions and passes saying nothing. + assert len(question.parts) == _expected_parts(fields, number) for index, part in enumerate(question.parts, start=1): + if f"q{number}.p{index}.text" not in fields: + # The question's own solution, written where every part is answered + # already: last, and holding nothing but that solution. + assert part.text == "" + assert part.worked_solution == fields[f"q{number}.solution"]["value"] + continue assert part.text == fields[f"q{number}.p{index}.text"]["value"] # A part's own solution, or the question's where it has none of its own. solution = fields.get( @@ -583,6 +611,27 @@ def test_build_refuses_a_field_naming_an_image_that_is_not_there( assert not (tmp_path / "out").exists() +@needs_compiler +def test_render_leaves_out_a_figure_that_is_not_there( + tmp_path: Path, monkeypatch +) -> None: + """A draft is rendered to look at, and a figure yet to be found is one such fault. + + The compiler drops the reference and typesets the rest, which is what `build` + refuses to upload and what a reviewer wants to see. + """ + monkeypatch.chdir(tmp_path) + _built(FIGURE, tmp_path) + (tmp_path / "figure.png").unlink() + + result = CliRunner().invoke(cli, ["render"]) + + assert result.exit_code == 0, result.output + written = sorted((tmp_path / "out").glob("*.pdf")) + assert len(written) == 1 + assert written[0].stat().st_size + + @needs_compiler @pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) def test_render_writes_one_pdf_per_question( From f7c8fad2f611fa03f12c7f918f4f3b128816f530 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 14:51:49 +0100 Subject: [PATCH 5/6] implement: Build only after a clean validate (t35) --- in2lambda/draft/export.py | 28 +++++++++++++++----- in2lambda/validation/pdf/__init__.py | 31 +++++++++++++++++------ tests/test_draft.py | 38 ++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py index 1fd88af..c139f70 100644 --- a/in2lambda/draft/export.py +++ b/in2lambda/draft/export.py @@ -167,24 +167,26 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: under a heading naming each, so what comes out is what a student would be shown. The checks are not run first: looking at a draft is how what they found gets fixed. Nor does a figure that is not beside the draft stop a question being looked at - - the compiler drops the reference and typesets the rest of it. + the compiler drops the reference and typesets the rest of it - or a question the + compiler gives up on altogether stop the rest of the draft being written out. Args: directory: Where the ``draft.json`` to render is. output_dir: Where to write the PDFs, named as the export names its questions. Returns: - The PDF written for each question, in question order. + The PDF written for each question that was rendered, in question order. Raises: ConversionToolsMissing: pandoc or xelatex is not installed. - CompileFailed: a question produced no PDF at all, or the compiler did not - finish with it. + CompileFailed: no question could be rendered at all, so there is nothing to + look at. SourceError: the draft is missing, is not one of ours, or was written from markdown that has changed since. Warns: - UserWarning: once per LaTeX error in a question that was rendered anyway. + UserWarning: once per LaTeX error in a question that was rendered anyway, and + once for a question the compiler gave up on while others rendered. """ if missing := pdf.missing_tools(): raise ConversionToolsMissing( @@ -193,6 +195,7 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: draft, _ = frozen(directory) written = [] + refused = [] for index, question in enumerate(as_set(draft, directory).questions): stem = _question_stem(index, _question_title(question, index)) output = Path(output_dir) / f"{stem}.pdf" @@ -200,9 +203,22 @@ def render(directory: str = ".", output_dir: str = "out") -> list[Path]: # through, and so that a question with nothing written in it is still a page. heading = f"Question {index + 1}" fields = [(heading, f"# {heading}")] + _fields(question, index + 1) - for problem in pdf.render(fields, question.images, output): + try: + problems = pdf.render(fields, question.images, output) + except pdf.CompileFailed as failed: + refused.append(str(failed)) + continue + for problem in problems: warnings.warn(str(problem), stacklevel=2) written.append(output) + if refused and not written: + # Nothing at all to look at, which is a failed run rather than a fault in one + # question of it, so it is said the way a draft that cannot be read is. + raise pdf.CompileFailed("; ".join(refused)) + for failure in refused: + # One question TeX cannot finish is a fault in that question like any other, and + # the ones that do compile are still what the draft is being rendered for. + warnings.warn(failure, stacklevel=2) return written diff --git a/in2lambda/validation/pdf/__init__.py b/in2lambda/validation/pdf/__init__.py index 61d17cc..fb574d5 100644 --- a/in2lambda/validation/pdf/__init__.py +++ b/in2lambda/validation/pdf/__init__.py @@ -48,6 +48,9 @@ ) """One ``-file-line-error`` line. The file is only ``set.tex`` for the set's own text.""" +_ABORTED = re.compile(r"^! (.+)$", re.MULTILINE) +r"""An error xelatex printed with no file and line to it - see :func:`_aborted`.""" + _IMAGE = re.compile(r"(!\[[^\]]*\]\()([^)]*)(\))") """A markdown image with its path apart, so that the path can be rewritten or dropped.""" @@ -56,10 +59,10 @@ class CompileFailed(SourceError): - """The pipeline produced nothing: pandoc refused the set, xelatex wrote no PDF, or - neither finished. + """The pipeline produced nothing at all. - Not a problem in one field, since there is no generated LaTeX to trace an error back + Either pandoc refused the set, or xelatex wrote no PDF, or neither finished. Not a + problem in one field, since there is no generated LaTeX to trace an error back through, so it is raised rather than reported - as a `SourceError`, which is what the command line turns into a message rather than a traceback. """ @@ -130,13 +133,14 @@ def render( latex, log = _compile(fields, images, work) problems = _reported(log, _locations(latex)) if not (compiled := work / "set.pdf").is_file(): + # Nothing traced back to a field is the emergency-stop case: xelatex gave up + # where it could name no line, so the log's own `!` lines are the only + # account there is of why nothing was typeset, and saying none would leave + # the refusal naming no cause at all. + said = [str(problem) for problem in problems] or _aborted(log) raise CompileFailed( f"xelatex produced no PDF of {output.name}" - + ( - ": " + "; ".join(str(problem) for problem in problems) - if problems - else "." - ) + + (": " + "; ".join(said) if said else ".") ) output.parent.mkdir(parents=True, exist_ok=True) shutil.copy(compiled, output) @@ -248,6 +252,17 @@ def _marked_document(fields: list[tuple[str, str]], available: set[str]) -> str: return "\n".join(blocks) +def _aborted(log: str) -> list[str]: + r"""Everything the xelatex log says went wrong without saying where. + + ``-file-line-error`` rewrites an error that happened at a line of a file, which is + what :func:`_reported` reads. An error that happened as the input ran out - + ``File ended while scanning use of \frac`` - has no such line, and xelatex prints it + the old way, so it is only ever in the log behind a ``!``. + """ + return [error.strip() for error in _ABORTED.findall(log)] + + def _locations(latex: str) -> list[tuple[int, str]]: """Each marker in the generated LaTeX as the line it is on and the field it names.""" return [ diff --git a/tests/test_draft.py b/tests/test_draft.py index 1896886..9626f8d 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -632,6 +632,44 @@ def test_render_leaves_out_a_figure_that_is_not_there( assert written[0].stat().st_size +@needs_compiler +def test_render_writes_the_questions_beside_one_tex_cannot_finish( + tmp_path: Path, monkeypatch +) -> None: + """Maths a brace is missing out of makes TeX give up where it stands. + + That is one question of the draft unrendered, and it is said as such: the rest is + still written out, since a draft whose faults are being fixed is exactly the one + somebody is looking at. + """ + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + _built(TWO_QUESTIONS, tmp_path) + replaced = CliRunner().invoke( + cli, + [ + "draft", + "field", + "replace", + "q1.solution", + "$Q = \\pi d^2 v / 4$", + "$\\frac{1$", + ], + ) + assert replaced.exit_code == 0, replaced.output + + result = CliRunner().invoke(cli, ["render"]) + + assert result.exit_code == 0, result.output + # The second question, which has nothing wrong with it. + written = sorted((tmp_path / "out").glob("*.pdf")) + assert len(written) == 1 + assert written[0].stat().st_size + # Why the first one is not there, rather than only that xelatex wrote no PDF: the + # log's own account of it is all there is when it stopped before reaching a field. + assert "File ended while scanning use of \\frac" in result.output + + @needs_compiler @pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) def test_render_writes_one_pdf_per_question( From 8cc5fd60721c50cedcc6fcc66ed5214dbdfdf714 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Sun, 20 Sep 2026 15:00:50 +0100 Subject: [PATCH 6/6] implement: Build only after a clean validate (t35) --- in2lambda/draft/export.py | 12 ++++++++++-- tests/fixtures/drafts/README.md | 4 +++- .../drafts/question_without_parts/commands.json | 9 +++++++++ .../drafts/question_without_parts/expected.json | 14 ++++++++++++++ .../drafts/question_without_parts/source.md | 1 + tests/test_draft.py | 14 ++++++++++---- 6 files changed, 47 insertions(+), 7 deletions(-) create mode 100644 tests/fixtures/drafts/question_without_parts/commands.json create mode 100644 tests/fixtures/drafts/question_without_parts/expected.json create mode 100644 tests/fixtures/drafts/question_without_parts/source.md diff --git a/in2lambda/draft/export.py b/in2lambda/draft/export.py index c139f70..911fb0f 100644 --- a/in2lambda/draft/export.py +++ b/in2lambda/draft/export.py @@ -59,8 +59,10 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: question was written without parts, it becomes a part of its own holding nothing but that solution. That is the rule :meth:`~in2lambda.api.question.Question.add_solution` applies, so a draft - exports as the same sheet converted by `in2lambda convert` does. A block marked - ignore is in no question: it is the source's, not the set's. + exports as the same sheet converted by `in2lambda convert` does. A question + written with neither parts nor a solution holds one part with nothing in it, + which is the question as the draft has it. A block marked ignore is in no + question: it is the source's, not the set's. Examples: >>> from in2lambda.draft.export import as_set @@ -100,6 +102,12 @@ def as_set(draft: dict[str, Any], directory: str = ".") -> Set: for part_of in question.parts: if not part_of.worked_solution: part_of.worked_solution = fields[written]["value"] + if not question.parts: + # A question whose parts are yet to be written is still exported, and an + # empty part is what it holds: `json_convert` leaves a question with no + # parts at all carrying the template's own placeholder wording, which is + # wording no field of the draft holds. + question.parts.append(Part()) # As the export refers to them: beside the draft, since that is where a command # naming a file names one. Whether the file is there is `build`'s question, not # asked here, so that a draft can be rendered while its figures are being found. diff --git a/tests/fixtures/drafts/README.md b/tests/fixtures/drafts/README.md index e3fb0a6..4f445a8 100644 --- a/tests/fixtures/drafts/README.md +++ b/tests/fixtures/drafts/README.md @@ -28,4 +28,6 @@ into `media/` - the only folder here with a file the fields point at. `question_solution_beside_part_solutions` is the only one filled in by a spec rather than by commands one at a time, and the only one whose question has two parts: both are answered by the spec's own solutions, so the `question solution` after it answers nothing, and the export carries -it as a part of its own rather than dropping the wording. +it as a part of its own rather than dropping the wording. `question_without_parts` is a question +and nothing else, which the checks have nothing to say about: it is here because a question with +no parts is what the export has to write out as an empty part rather than as the template's. diff --git a/tests/fixtures/drafts/question_without_parts/commands.json b/tests/fixtures/drafts/question_without_parts/commands.json new file mode 100644 index 0000000..10b9471 --- /dev/null +++ b/tests/fixtures/drafts/question_without_parts/commands.json @@ -0,0 +1,9 @@ +[ + { + "args": { + "text": "b1" + }, + "by": "tests", + "command": "question add" + } +] diff --git a/tests/fixtures/drafts/question_without_parts/expected.json b/tests/fixtures/drafts/question_without_parts/expected.json new file mode 100644 index 0000000..4cc1b0a --- /dev/null +++ b/tests/fixtures/drafts/question_without_parts/expected.json @@ -0,0 +1,14 @@ +{ + "q1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 1, + 1 + ] + ], + "value": "Find the pressure at the bottom of a tank of water $h$ deep." + } +} diff --git a/tests/fixtures/drafts/question_without_parts/source.md b/tests/fixtures/drafts/question_without_parts/source.md new file mode 100644 index 0000000..919165f --- /dev/null +++ b/tests/fixtures/drafts/question_without_parts/source.md @@ -0,0 +1 @@ +Find the pressure at the bottom of a tank of water $h$ deep. diff --git a/tests/test_draft.py b/tests/test_draft.py index 9626f8d..27349dc 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -59,7 +59,9 @@ def _expected_parts(fields: dict[str, Any], number: int) -> int: One per ``qN.pM.text``, and one more where ``qN.solution`` is written beside a solution for every part there is: nothing is left for it to answer, so it is a part - of its own, as `in2lambda convert` writes one. + of its own, as `in2lambda convert` writes one. A question with no parts written for + it at all is one empty part, since a question exported holding none carries the + template's placeholder wording instead. """ written = [ int(found[2]) @@ -67,7 +69,8 @@ def _expected_parts(fields: dict[str, Any], number: int) -> int: if (found := PART.fullmatch(key)) and int(found[1]) == number ] answered = all(f"q{number}.p{part}.solution" in fields for part in written) - return len(written) + (answered and f"q{number}.solution" in fields) + parts = len(written) + (answered and f"q{number}.solution" in fields) + return parts or 1 def _reported(folder: Path) -> list[dict[str, Any]]: @@ -568,9 +571,12 @@ def test_build_follows_the_report(folder: Path, tmp_path: Path, monkeypatch) -> for index, part in enumerate(question.parts, start=1): if f"q{number}.p{index}.text" not in fields: # The question's own solution, written where every part is answered - # already: last, and holding nothing but that solution. + # already: last, and holding nothing but that solution. Or, where the + # question has no solution either, the empty part a question with no + # parts written for it exports as. assert part.text == "" - assert part.worked_solution == fields[f"q{number}.solution"]["value"] + solution = fields.get(f"q{number}.solution") + assert part.worked_solution == (solution["value"] if solution else "") continue assert part.text == fields[f"q{number}.p{index}.text"]["value"] # A part's own solution, or the question's where it has none of its own.