Render review questions through in2lambda.draft.export.render

README.md

@@ -158,22 +158,20 @@ difference it reports does not change the exit code.
 default, builds as soon as the checks report no error. `sample` shows a few questions —
 `--sample N`, three by default, the ones a fixing round or an edit wrote first — and
 `per-question` shows every question. In either mode the run writes no zip and prints
-each question with the lines of the frozen source its fields were copied from:
+each question with the PDF it was rendered to and the lines of the frozen source its
+fields were copied from:
 
 ```
-render    in2lambda render is not there yet, so the review names each question by the lines of the source it was built from instead
+render    2 questions to /home/me/out/render
 review    mode sample, 2 of 2 questions waiting:
-  q1 pending: not rendered, /home/me/sheets/sheet.md lines 5-5, 7-7
-  q2 pending: not rendered, /home/me/sheets/sheet.md lines 13-13
+  q1 pending: /home/me/out/render/question_000_Question_1.pdf, /home/me/sheets/sheet.md lines 5-5, 7-7
+  q2 pending: /home/me/out/render/question_001_Question_2.pdf, /home/me/sheets/sheet.md lines 13-13
   answer with `in2lambda-agent review approve Q --cache /home/me/.in2lambda-agent`, …
 ```
 
-Those two lines are what a run prints today. in2lambda writes a PDF per question from
-`in2lambda.draft.export.render`, and the agent does not call it yet (ticket t25), so
-nothing writes the pages and each question reads `not rendered`. Once the agent calls
-it, the same run writes `q1.pdf` and the rest under `--out`'s `render` folder, and the
-two lines name those files instead — `render    2 questions to /home/me/out/render`,
-and `q1 pending: /home/me/out/render/q1.pdf, /home/me/sheets/sheet.md lines 5-5, 7-7`.
+Rendering the questions needs pandoc and xelatex. Without them the `render` line
+carries in2lambda's own message, each question reads `not rendered`, and the review
+goes on with the lines of the frozen source.
 
 The reviewer answers from the command line, in as many commands as they like:
 

docs/how-it-works.md

@@ -38,7 +38,7 @@ per fixing round, so those two names repeat.
 | `coverage` | `in2lambda.draft.execute` with `in2lambda.draft.spec_command` | the layer 1 fields of the draft |
 | `validate` | `in2lambda.draft.report.validate` | the report inside the draft |
 | `fix` | `in2lambda.source.show`, then `in2lambda.draft.execute` once per command | the fields and the log of the draft |
-| `render` | `in2lambda.draft.export.render`, which the agent does not call yet | `OUT/render/q1.pdf`, one PDF per question, once it does |
+| `render` | `in2lambda.draft.export.render` | `OUT/render/question_000_Question_1.pdf`, one PDF per question |
 | `review` | none | `CACHE/review.json` |
 | `build` | `in2lambda.draft.export.build` | `OUT/set.zip` and the set's JSON folder |
 
@@ -148,18 +148,17 @@ no rounds left to answer the note with: the run was --rounds 0
 
 ### `render`
 
-The stage runs only where the run is in review. It calls in2lambda's renderer, which
-writes one PDF per question under `OUT/render`. in2lambda defines that renderer at
-`in2lambda.draft.export.render(draft, output_dir)`, and `package.render` looks it up
-at `in2lambda.draft.render`, so the lookup finds nothing and the stage raises
-`RenderUnavailable` on every review today. Ticket t25 looks the renderer up where
-in2lambda defines it. The stage prints one of two messages:
+The stage runs only where the run is in review. It calls
+`in2lambda.draft.export.render(draft, output_dir)`, which compiles one PDF per
+question under `OUT/render` as Lambda Feedback's own PDF generator compiles it. Each
+file is named after the question's place in the set and its title:
+`question_000_Question_1.pdf`. The stage prints one of two messages:
 
-* `in2lambda render is not there yet, so the review names each question by the lines
-  of the source it was built from instead` — the lookup found no renderer. This is
-  what every review prints today, and each question reads `not rendered`.
-* `2 questions to /home/me/out/render` — the PDFs were written, which a review prints
-  once the agent calls the renderer.
+* `2 questions to /home/me/out/render` — the PDFs were written, and each question's
+  line in the listing names its file.
+* in2lambda's own message, such as `Rendering questions needs xelatex.` — pandoc or
+  xelatex is missing, or no question compiled. The review goes on, and each question
+  reads `not rendered`.
 
 ### `review`
 
@@ -171,8 +170,8 @@ The stage prints one of six messages:
 
   ```
   mode sample, 2 of 2 questions waiting:
-    q1 pending: not rendered, /home/me/sheets/sheet.md lines 5-5, 7-7
-    q2 rejected: not rendered, /home/me/sheets/sheet.md lines 13-13 — the solution answers (a), not (b)
+    q1 pending: /home/me/out/render/question_000_Question_1.pdf, /home/me/sheets/sheet.md lines 5-5, 7-7
+    q2 rejected: /home/me/out/render/question_001_Question_2.pdf, /home/me/sheets/sheet.md lines 13-13 — the solution answers (a), not (b)
     answer with `in2lambda-agent review approve Q --cache /home/me/.in2lambda-agent`, `review reject Q --note "..."` or `review edit FIELD OLD NEW`
   ```
 

in2lambda_agent/package.py

@@ -14,7 +14,7 @@
     in2lambda.draft.execute(
         {"command": name, "args": {...}, "by": by}, draft) -> str
     in2lambda.draft.report.validate(draft) -> list[Finding]
-    in2lambda.draft.render(draft, out_dir) -> dict  # not there yet
+    in2lambda.draft.export.render(draft, output_dir) -> list[Path]
     in2lambda.draft.export.build(draft, output_dir) -> Path
 
 A draft lives beside the file it was frozen from and is named after it:
@@ -24,6 +24,7 @@
 """
 
 import json
+import re
 import warnings
 from dataclasses import dataclass, field
 from os.path import relpath
@@ -61,6 +62,10 @@
 )
 """The draft commands a report is fixed with, named as the log names them."""
 
+RENDERED = re.compile(r"question_(\d+)_")
+"""How in2lambda names the PDF it writes for a question: the question's place in
+the set, counting from zero, so that a stack of them reads in order."""
+
 
 class SpecRejected(ValueError):
     """in2lambda would not run a spec, and says why."""
@@ -70,10 +75,6 @@ class CommandRefused(ValueError):
     """in2lambda would not run a draft command, and says why."""
 
 
-class RenderUnavailable(RuntimeError):
-    """in2lambda has no render command yet, so there are no pages to show."""
-
-
 class BuildRefused(ValueError):
     """in2lambda would not write a validated draft out, and says why."""
 
@@ -371,23 +372,25 @@ def render(draft: Path, out_dir: Path) -> dict[str, Path]:
         out_dir: Where to write the PDFs.
 
     Returns:
-        The PDF written for each question, keyed as `questions` keys it.
+        The PDF written for each question, keyed as `questions` keys it. A
+        question the compiler gave up on while the rest rendered has no PDF and
+        is not in it, which is why the key comes from the file's name rather
+        than from its place in the list.
 
     Raises:
-        RenderUnavailable: in2lambda has no render command yet.
-        CommandRefused: the draft cannot be rendered, and in2lambda says why.
+        CommandRefused: the pages cannot be compiled — pandoc or xelatex is
+            missing, or in2lambda says why.
     """
-    renderer = getattr(in2lambda.draft, "render", None)
-    if renderer is None:
-        raise RenderUnavailable(
-            "in2lambda render is not there yet, so the review names each "
-            "question by the lines of the source it was built from instead"
-        )
     try:
-        written = renderer(str(draft), str(out_dir))
+        written = in2lambda.draft.export.render(str(draft), str(out_dir))
     except SourceError as error:
         raise CommandRefused(str(error)) from None
-    return {key: Path(path) for key, path in written.items()}
+    keyed = {}
+    for path in written:
+        numbered = RENDERED.match(Path(path).name)
+        if numbered:
+            keyed[f"q{int(numbered.group(1)) + 1}"] = Path(path)
+    return keyed
 
 
 def layers(draft: Path) -> dict[str, int]:

in2lambda_agent/pipeline.py

@@ -724,9 +724,12 @@ def _render(draft: Path, out_dir: Path) -> tuple[dict[str, Path], str]:
     """Renders the draft's questions, or says why there are no pages to show."""
     try:
         rendered = package.render(draft, out_dir / "render")
-    except package.RenderUnavailable as unavailable:
-        return {}, str(unavailable)
-    return rendered, f"{len(rendered)} questions to {out_dir / 'render'}"
+    except package.CommandRefused as refused:
+        return {}, str(refused)
+    return rendered, (
+        f"{len(rendered)} question{'' if len(rendered) == 1 else 's'} "
+        f"to {out_dir / 'render'}"
+    )
 
 
 def _relist(waiting: Review, result: RunResult, keys: list[str]) -> None:

tests/test_package.py

@@ -5,8 +5,10 @@
 import warnings
 from pathlib import Path
 
-import in2lambda.draft
+import in2lambda.draft.export
 import pytest
+from in2lambda.source import ConversionToolsMissing
+from in2lambda.validation.pdf import missing_tools
 
 from in2lambda_agent import package
 
@@ -139,23 +141,73 @@ def test_the_frozen_source_is_named_from_the_draft(draft):
     assert package.frozen_source(draft).is_file()
 
 
-def test_rendering_says_that_in2lambda_has_no_render_yet(draft, tmp_path):
-    with pytest.raises(package.RenderUnavailable, match="in2lambda render"):
-        package.render(draft, tmp_path / "render")
-
-
 def test_rendering_names_the_pdf_written_for_each_question(
     draft, tmp_path, monkeypatch
 ):
-    # What `in2lambda render` will do when it is there, so that the review
-    # reads the same either way.
+    out = tmp_path / "render"
+    # in2lambda numbers each file with the question's place in the set, counting
+    # from zero, which is what the key comes from: a question the compiler gave
+    # up on is left out of the list, so counting the list would number the rest
+    # wrongly.
     monkeypatch.setattr(
-        in2lambda.draft,
+        in2lambda.draft.export,
         "render",
-        lambda directory, out: {"q1": f"{out}/q1.pdf"},
-        raising=False,
+        lambda written, directory: [
+            Path(directory) / "question_001_Question_2.pdf",
+            Path(directory) / "question_000_Question_1.pdf",
+        ],
     )
 
-    assert package.render(draft, tmp_path / "render") == {
-        "q1": tmp_path / "render" / "q1.pdf"
+    assert package.render(draft, out) == {
+        "q1": out / "question_000_Question_1.pdf",
+        "q2": out / "question_001_Question_2.pdf",
     }
+
+
+def test_rendering_that_cannot_compile_is_refused_with_in2lambdas_reason(
+    draft, tmp_path, monkeypatch
+):
+    def missing(written, directory):
+        raise ConversionToolsMissing("Rendering questions needs xelatex.")
+
+    monkeypatch.setattr(in2lambda.draft.export, "render", missing)
+
+    with pytest.raises(package.CommandRefused, match="needs xelatex"):
+        package.render(draft, tmp_path / "render")
+
+
+@pytest.mark.skipif(bool(missing_tools()), reason="needs pandoc and xelatex")
+def test_rendering_writes_a_pdf_for_each_question_of_the_draft(draft, tmp_path):
+    rendered = package.render(draft, tmp_path / "render")
+
+    assert list(rendered) == ["q1", "q2"]
+    assert all(path.parent == tmp_path / "render" for path in rendered.values())
+    assert all(path.is_file() for path in rendered.values())
+
+
+@pytest.mark.skipif(bool(missing_tools()), reason="needs pandoc and xelatex")
+def test_a_question_the_compiler_gives_up_on_leaves_the_rest_keyed_as_they_were(
+    draft, tmp_path
+):
+    # TeX stops on a file it cannot find before it has typeset anything, so q1
+    # has no page while q2 still does. in2lambda leaves it out of the list it
+    # returns and names the files it did write after the question's place in
+    # the set, which is what the key must come from: counting the list would
+    # hand q2's page to q1 and leave q2 reading `not rendered`.
+    package.command(
+        draft,
+        "field replace",
+        {
+            "field": "q1.text",
+            "old": r"A ball is thrown straight up at $20\,\mathrm{m/s}$.",
+            "new": r"\input{no-such-file-at-all}",
+        },
+    )
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        rendered = package.render(draft, tmp_path / "render")
+
+    assert rendered == {"q2": tmp_path / "render" / "question_001_Question_2.pdf"}
+    assert rendered["q2"].is_file()
+    assert not (tmp_path / "render" / "question_000_Question_1.pdf").exists()

tests/test_pipeline.py

@@ -7,9 +7,10 @@
 import zipfile
 from pathlib import Path
 
-import in2lambda.draft
+import in2lambda.draft.export
 import pytest
 from conftest import PNG, FakeBackend, FakeMathpix
+from in2lambda.source import ConversionToolsMissing
 from in2lambda.validation.pdf import missing_tools
 
 from in2lambda_agent import package, pair, pipeline
@@ -930,23 +931,30 @@ def reviewed(sheets, tmp_path, mode="sample", **given):
     )
 
 
+@pytest.mark.skipif(
+    bool(missing_tools()), reason="rendering the questions needs pandoc and xelatex"
+)
 def test_a_review_stops_the_run_with_the_questions_listed_and_no_zip(
     sheets, tmp_path
 ):
     result = reviewed(sheets, tmp_path)
     stages = {stage.name: stage.message for stage in result.stages}
+    pages = tmp_path / "out" / "render"
 
     assert [stage.name for stage in result.stages][-3:] == [
         "validate",
         "render",
         "review",
     ]
-    # Each question by its key, the PDF where one was rendered, and the lines
-    # of the frozen source it was built from.
-    assert f"q1 pending: not rendered, {sheets / 'sheet.md'} lines 5-5, 7-7" in (
-        stages["review"]
-    )
-    assert "in2lambda render is not there yet" in stages["render"]
+    # Each question by its key, the PDF it was rendered to, and the lines of
+    # the frozen source it was built from.
+    assert stages["render"] == f"2 questions to {pages}"
+    assert (
+        f"q1 pending: {pages / 'question_000_Question_1.pdf'}, "
+        f"{sheets / 'sheet.md'} lines 5-5, 7-7"
+    ) in stages["review"]
+    assert (pages / "question_000_Question_1.pdf").is_file()
+    assert f"q2 pending: {pages / 'question_001_Question_2.pdf'}, " in stages["review"]
     assert "in2lambda-agent review approve Q" in stages["review"]
     # Nothing built, and no run recorded: the run is not over.
     assert result.zip_path is None
@@ -1002,7 +1010,7 @@ def test_a_review_of_a_question_a_literal_wrote_lists_the_lines_it_has(
         [13, 13],
         [14, 14],
     ]
-    assert "q2 pending: not rendered" in stages["review"]
+    assert "q2 pending: " in stages["review"]
     assert "lines 13-13, 14-14" in stages["review"]
     assert result.zip_path is None
 
@@ -1017,19 +1025,45 @@ def test_a_sample_shows_a_few_questions_and_per_question_shows_them_all(
     assert [one.key for one in every.review.questions] == ["q1", "q2"]
 
 
-def test_the_review_names_the_pdf_a_render_wrote(sheets, tmp_path, monkeypatch):
+def test_a_review_carries_on_when_the_pages_cannot_be_compiled(
+    sheets, tmp_path, monkeypatch
+):
+    # A machine without xelatex, which is where most of the corpus is read: the
+    # render stage says what in2lambda said, and the reviewer gets the same
+    # listing named by lines instead of pages.
+    def missing(draft, directory):
+        raise ConversionToolsMissing("Rendering questions needs xelatex.")
+
+    monkeypatch.setattr(in2lambda.draft.export, "render", missing)
+
+    result = reviewed(sheets, tmp_path)
+    stages = {stage.name: stage.message for stage in result.stages}
+
+    assert stages["render"] == "Rendering questions needs xelatex."
+    assert f"q1 pending: not rendered, {sheets / 'sheet.md'} lines 5-5, 7-7" in (
+        stages["review"]
+    )
+    assert result.zip_path is None
+    assert json.loads((tmp_path / "cache" / "review.json").read_text())["mode"] == (
+        "sample"
+    )
+
+
+def test_the_render_line_counts_one_page_as_one_question(tmp_path, monkeypatch):
+    # A sheet with one question, or a set the compiler gave up on all but one
+    # of: either way the line reads as English rather than `1 questions`.
     monkeypatch.setattr(
-        in2lambda.draft,
+        package,
         "render",
-        lambda directory, out: {"q1": f"{out}/q1.pdf", "q2": f"{out}/q2.pdf"},
-        raising=False,
+        lambda draft, out: {"q1": out / "question_000_Question_1.pdf"},
     )
 
-    result = reviewed(sheets, tmp_path)
-    stages = {stage.name: stage.message for stage in result.stages}
+    rendered, message = pipeline._render(
+        tmp_path / "sheet.draft.json", tmp_path / "out"
+    )
 
-    assert stages["render"] == f"2 questions to {tmp_path / 'out' / 'render'}"
-    assert str(tmp_path / "out" / "render" / "q1.pdf") in stages["review"]
+    assert list(rendered) == ["q1"]
+    assert message == f"1 question to {tmp_path / 'out' / 'render'}"
 
 
 def test_approving_every_question_builds_the_set_and_records_the_review(