diff --git a/README.md b/README.md index 0d85aa1..b22d333 100644 --- a/README.md +++ b/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: diff --git a/docs/how-it-works.md b/docs/how-it-works.md index ad22e5d..a0fdf74 100644 --- a/docs/how-it-works.md +++ b/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` ``` diff --git a/in2lambda_agent/package.py b/in2lambda_agent/package.py index 8ad0b4f..7778aec 100644 --- a/in2lambda_agent/package.py +++ b/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]: diff --git a/in2lambda_agent/pipeline.py b/in2lambda_agent/pipeline.py index d57f05f..5440952 100644 --- a/in2lambda_agent/pipeline.py +++ b/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: diff --git a/tests/test_package.py b/tests/test_package.py index 64ed67f..25a5b45 100644 --- a/tests/test_package.py +++ b/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() diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index d07639c..3b22aca 100644 --- a/tests/test_pipeline.py +++ b/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(