diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d0de84..3bc651b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,4 +10,5 @@ - A draft is filled in by `in2lambda draft question add`, `in2lambda draft part add QUESTION` and `in2lambda draft question solution QUESTION`. Each takes `--text` to copy the wording out of the frozen source, as a block id such as `b3` or as lines such as `s10:14`, or `--literal TEXT` where the source does not say it in a form the field can take, which records the field as edited and written by layer 4 rather than 3. Question and part numbers are worked out from the fields already written rather than given, so a replay arrives at the same ids. `in2lambda draft split block BLOCK AT` cuts a block the parser made one of two things into `b3a` and `b3b`, so that each half can be quoted on its own. A command writing a field that is already written, or quoting lines another field was taken from, is refused naming both fields. - `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. - 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/docs/source/spec.md b/docs/source/spec.md index 3a4f7bb..8134a71 100644 --- a/docs/source/spec.md +++ b/docs/source/spec.md @@ -7,11 +7,11 @@ every question comes out of the source rather than being retyped: ```bash $ in2lambda source add questions.docx $ in2lambda spec run spec.yaml -b6 is in no field. +b6 (lines 12-13) is in no field and not marked ignore. ``` -The last line is the point of it: a spec run reports every block it made nothing of, so what is -left to account for is in front of you rather than quietly missing. +The last line is the point of it: a spec run reports every block it made nothing of, naming the +lines it is, so what is left to account for is in front of you rather than quietly missing. The fields a draft holds belong to the spec that wrote them, so a spec is run over a draft once. Running an edited one again is refused; freeze the document afresh and run it, which is two @@ -91,9 +91,10 @@ the solutions are, and what each of them answers. ## What it writes -Each question is `q1`, `q2` and so on in the order they appear, and each of its parts `q1.a`, -`q1.b`. So a spec fills in `q1.text`, `q1.a.text`, `q1.a.solution` and, for a question answered -as a whole, `q1.solution`. Every one of them records the lines it was copied from, and that a +Each question is `q1`, `q2` and so on in the order they appear, and each of its parts `q1.p1`, +`q1.p2`. So a spec fills in `q1.text`, `q1.p1.text`, `q1.p1.solution` and, for a question +answered as a whole, `q1.solution`. They are the names the `in2lambda draft` commands give out +as well, so a draft filled in either way is the same draft. Every one of them records the lines it was copied from, and that a spec wrote it. The spec is recorded in the draft's log with its hash, so `in2lambda draft replay` rebuilds the diff --git a/in2lambda/draft/__init__.py b/in2lambda/draft/__init__.py index cd88416..af86188 100644 --- a/in2lambda/draft/__init__.py +++ b/in2lambda/draft/__init__.py @@ -16,6 +16,7 @@ from typing import Any import in2lambda.spec +from in2lambda.draft.report import checks, overlapping, uncovered from in2lambda.source import ( DRAFT, SourceError, @@ -148,8 +149,11 @@ def record( "in2lambda source add --start-over to begin the draft again." ) for filled, field in draft["fields"].items(): + # Each of the field's ranges on its own, so that the refusal names the one in + # the way: a field edited by hand can be quoted from several, and the rest of + # them may be lines nobody wants. for taken in field["ranges"]: - if any(taken[0] <= end and start <= taken[1] for start, end in ranges): + if overlapping(ranges, [taken]): raise AlreadyFilled( f"Lines {taken[0]}-{taken[1]} are where {filled} came from, so " f"they cannot also be {key}. Run in2lambda source show to see " @@ -251,6 +255,9 @@ def apply( written = handler(draft, markdown, entry["args"], entry["by"], directory) # After the handler, so a command that was refused is not recorded as having run. draft["log"].append(entry) + # A report is about the draft as it was, so the command that changes it takes the + # report with it rather than leaving one that describes something else. + draft.pop("report", None) return written @@ -306,6 +313,10 @@ def replay(directory: str = ".") -> None: } for entry in draft["log"]: apply(rebuilt, markdown, entry, directory) + # The one thing in a draft that no command wrote: the checks did, over the draft the + # commands left, so rebuilding it is running them again rather than copying it. + if "report" in draft: + rebuilt["report"] = checks(rebuilt) path = Path(directory) / DRAFT if serialise(rebuilt) != path.read_bytes(): @@ -316,32 +327,6 @@ def replay(directory: str = ".") -> None: ) -def coverage(draft: dict[str, Any]) -> list[str]: - """The blocks of a draft that nothing has made anything of yet. - - Args: - draft: The draft to look over. - - Returns: - The ids of the blocks that are in no field and have not been ignored, in - document order. A spec run prints these: they are what is left to account for, - and an empty list is the whole document spoken for. - """ - ranges = [ - line_range - for field in draft["fields"].values() - for line_range in field["ranges"] - ] - return [ - block["id"] - for block in draft["blocks"] - if f"{block['id']}.ignore" not in draft["fields"] - and not any( - start <= block["end"] and block["start"] <= end for start, end in ranges - ) - ] - - def _block(draft: dict[str, Any], block: str) -> dict[str, Any]: """One block of the frozen source, given the draft has one of that id. @@ -675,20 +660,21 @@ def _spec_run( # The blocks the selectors run over are the ones the parser makes of the source, and # a `split block` since has left the draft holding halves the parser never made. So # an ignored block is named and ranged from here rather than from the draft: the - # field then spans the whole of what was ignored, and coverage, which goes by lines - # as well as by name, counts each half of a split block as covered by it. + # field then spans the whole of what was ignored, and `uncovered`, which goes by the + # lines a field was taken from, counts each half of a split block as covered by it. elements = _elements(markdown) fields, ignored = in2lambda.spec.fields(spec, elements, markdown) for found in fields: record(draft, found.key, found.value, layer=1, ranges=found.ranges, by=by) lines = {block.id: [block.start, block.end] for block, _ in elements} - # The field `mark ignore` writes, so that coverage need not care which said so. + # The field `mark ignore` writes, so that `uncovered` need not care which said so. for block_id in ignored: record( draft, f"{block_id}.ignore", True, layer=1, ranges=[lines[block_id]], by=by ) # A spec writes a draft's worth of fields, so what it hands back is the other way - # round: what it made nothing of, which is what is left for anyone to act on. - if uncovered := coverage(draft): - return "\n".join(f"{block} is in no field." for block in uncovered) + # round: what it made nothing of, which is what is left for anyone to act on. Said + # in the words `in2lambda validate` says it in, since it is the same check. + if left_out := uncovered(draft): + return "\n".join(finding["message"] for finding in left_out) return "Every block is in a field or ignored." diff --git a/in2lambda/draft/report.py b/in2lambda/draft/report.py new file mode 100644 index 0000000..0367d54 --- /dev/null +++ b/in2lambda/draft/report.py @@ -0,0 +1,271 @@ +"""Checks a draft over as a whole, and writes what it finds into it. + +A draft is written one command at a time, and what a run of them left out is not +something any one command can see: a block nobody quoted, two fields taken from the same +lines, a question numbered 3 where there is no 2, a part with nothing answering it. So +the finished draft is looked over at once, and what the checks find is written into it as +its ``report``, which is what whoever is writing the draft - an agent or a person - reads +to find out what is left to do, without reading the draft itself. + +Everything here reports, never refuses: what the checks found may well be deliberate, and +deciding that is whoever is writing the draft's to do. Only what is in the draft is +looked at - its blocks, its field keys, their ranges and their values - because what the +text of a question says is `in2lambda.validation`'s, at export. +""" + +import re +from pathlib import Path +from typing import Any + +from in2lambda.source import DRAFT, frozen, save + +Finding = dict[str, Any] +"""One thing a check found: ``{"check", "field", "ranges", "message"}``. + +``check`` is which check found it, ``field`` the block id or field key it is about, +``ranges`` the lines in question as ``[[start, end], ...]``, and ``message`` a sentence +naming all of that, so that a line of the report can be acted on by itself. +""" + +_NUMBERED = re.compile(r"((?:q\d+\.p)|q)(\d+)\.text") +"""A question's or a part's text, split into what numbers it and the number.""" + +_PART = re.compile(r"(q\d+)\.p\d+\.text") +"""A part's text, and the question it belongs to.""" + +_UNPLACED = float("inf") +"""Where a finding about no particular line sorts: after every finding about one.""" + + +def overlapping(ranges: list[list[int]], other: list[list[int]]) -> bool: + """Whether any line falls in both sets of line ranges. + + Args: + ranges: Line ranges, as ``[[start, end], ...]``, each end inclusive. + other: The ranges to test them against. + + Returns: + Whether the two sets share a line. + + Examples: + >>> from in2lambda.draft.report import overlapping + >>> overlapping([[5, 6]], [[6, 8]]) + True + >>> overlapping([[5, 6]], [[7, 8]]) + False + """ + return any( + taken[0] <= end and start <= taken[1] + for taken in other + for start, end in ranges + ) + + +def _where(ranges: list[list[int]]) -> str: + """The lines something covers, as a message names them, or "" if it covers none.""" + if not ranges: + return "" + return " (lines " + ", ".join(f"{start}-{end}" for start, end in ranges) + ")" + + +def _runs(lines: list[int]) -> list[list[int]]: + """Line numbers in order, grouped into the ranges they run in.""" + runs: list[list[int]] = [] + for line in lines: + if runs and runs[-1][1] == line - 1: + runs[-1][1] = line + else: + runs.append([line, line]) + return runs + + +def uncovered(draft: dict[str, Any]) -> list[Finding]: + """Blocks of the source that no field, and no `mark ignore`, accounts for. + + A block partly quoted is reported for the rest of it: a question taken from the first + line of a block leaves the other lines as much unaccounted for as a whole block would. + Blocks are accounted for by the lines the fields were taken from rather than by name, + so that a block `split block` has cut in two is covered by an ignore of the whole. + + Args: + draft: A draft, as `in2lambda.source.frozen` reads one. + + Returns: + One :data:`Finding` per block with lines nothing has made anything of, in + document order. `in2lambda.spec` reports through this as well as the checks do: + what a spec run left out is the same question asked the moment it finishes. + """ + claimed = { + line + for field in draft["fields"].values() + for start, end in field["ranges"] + for line in range(start, end + 1) + } + found = [] + for block in draft["blocks"]: + free = _runs( + [ + line + for line in range(block["start"], block["end"] + 1) + if line not in claimed + ] + ) + if free: + found.append( + { + "check": "uncovered", + "field": block["id"], + "ranges": free, + "message": f"{block['id']}{_where(free)} is in no field and not " + "marked ignore.", + } + ) + return found + + +def _overlaps(draft: dict[str, Any]) -> list[Finding]: + """Pairs of fields quoted from some of the same lines. + + No command writes such a pair - `record` refuses the second of them - so this is here + for a draft edited by hand, where one of the two fields is quoting the wrong thing. + """ + fields = draft["fields"] + keys = sorted(fields) + return [ + { + "check": "overlap", + "field": key, + "ranges": fields[key]["ranges"], + "message": f"{key}{_where(fields[key]['ranges'])} and {other}" + f"{_where(fields[other]['ranges'])} are taken from some of the same lines.", + } + for index, key in enumerate(keys) + for other in keys[index + 1 :] + if overlapping(fields[key]["ranges"], fields[other]["ranges"]) + ] + + +def _gaps(draft: dict[str, Any]) -> list[Finding]: + """Questions or parts numbered past one that was never written. + + Numbers are given out by `in2lambda.draft._next`, which leaves no gap, so this too is + a draft that was edited: a question renumbered, or one deleted out of the middle. + """ + numbered: dict[str, list[int]] = {} + for key in draft["fields"]: + if named := _NUMBERED.fullmatch(key): + numbered.setdefault(named[1], []).append(int(named[2])) + return [ + { + "check": "gap", + "field": f"{prefix}{missing}.text", + "ranges": [], + "message": f"There is no {prefix}{missing}.text, though " + f"{prefix}{max(numbers)}.text is written: the numbering skips it.", + } + for prefix, numbers in numbered.items() + for missing in range(1, max(numbers)) + if missing not in numbers + ] + + +def _without_solutions(draft: dict[str, Any]) -> list[Finding]: + """Parts that nothing in the draft answers. + + A part is answered by its own solution or by the solution of the question it belongs + to, since a sheet often writes one worked solution covering every part at once. + """ + fields = draft["fields"] + found = [] + for key in sorted(fields): + if (named := _PART.fullmatch(key)) is None: + continue + part = key.removesuffix(".text") + if f"{part}.solution" in fields or f"{named[1]}.solution" in fields: + continue + found.append( + { + "check": "no-solution", + "field": part, + "ranges": fields[key]["ranges"], + "message": f"{part}{_where(fields[key]['ranges'])} has no solution: " + f"neither {part}.solution nor {named[1]}.solution is written.", + } + ) + return found + + +def _empty(draft: dict[str, Any]) -> list[Finding]: + """Fields holding nothing, which is a quotation of the wrong lines or of none.""" + return [ + { + "check": "empty", + "field": key, + "ranges": field["ranges"], + "message": f"{key}{_where(field['ranges'])} is empty.", + } + for key, field in sorted(draft["fields"].items()) + if isinstance(field["value"], str) and not field["value"].strip() + ] + + +def checks(draft: dict[str, Any]) -> list[Finding]: + """Everything the checks find wrong with a draft, in the order of the source. + + Args: + draft: A draft, as `in2lambda.source.frozen` reads one. + + Returns: + One :data:`Finding` per thing found, earliest line first and then by what it is + about, with the findings about no particular line last. An empty list means the + draft covers its source once each, with nothing missing from its numbering. + + Examples: + >>> from in2lambda.draft.report import checks + >>> draft = { + ... "blocks": [{"id": "b1", "type": "paragraph", "start": 1, "end": 2}], + ... "fields": {}, + ... } + >>> [finding["message"] for finding in checks(draft)] + ['b1 (lines 1-2) is in no field and not marked ignore.'] + """ + found = ( + uncovered(draft) + + _overlaps(draft) + + _gaps(draft) + + _without_solutions(draft) + + _empty(draft) + ) + return sorted( + found, + key=lambda finding: ( + finding["ranges"][0][0] if finding["ranges"] else _UNPLACED, + finding["field"], + ), + ) + + +def validate(directory: str = ".") -> list[Finding]: + """Checks the draft in a directory over and writes the report into it. + + The report replaces whatever one is there, and is dropped again by the next command + that changes the draft: it describes the draft as it stood, and a report saying + something else is worse than none at all. + + Args: + directory: Where the ``draft.json`` to check is. + + Returns: + What the checks found, as it was written into the draft. + + Raises: + DraftMissing: there is no draft in that directory. + DraftUnreadable: what is there is not a draft anything here wrote. + SourceUnreadable: the markdown the draft names has moved, or is not text. + DraftExists: the markdown has changed since the draft was written from it, so + the lines the report named would not be the lines it was written about. + """ + draft, _ = frozen(directory) + draft["report"] = checks(draft) + save(Path(directory) / DRAFT, draft) + return draft["report"] diff --git a/in2lambda/main.py b/in2lambda/main.py index 0d163dd..9badce1 100644 --- a/in2lambda/main.py +++ b/in2lambda/main.py @@ -21,6 +21,7 @@ import rich_click as click import in2lambda.draft +import in2lambda.draft.report import in2lambda.filters import in2lambda.source from in2lambda.api.set import Set @@ -426,5 +427,22 @@ def spec_run(spec: str, by: str) -> None: click.echo(report) +@cli.command("validate") +def validate() -> None: + """Checks the draft in this directory over and writes the report into it. + + Reports 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. Finding something is not a failure: the report + is written into draft.json either way, and replaced by the next one. + """ + with _message_not_traceback(): + report = in2lambda.draft.report.validate() + for finding in report: + click.echo(finding["message"]) + if not report: + click.echo("Nothing to report.") + + if __name__ == "__main__": cli() diff --git a/in2lambda/source/__init__.py b/in2lambda/source/__init__.py index af03095..b300ab9 100644 --- a/in2lambda/source/__init__.py +++ b/in2lambda/source/__init__.py @@ -27,11 +27,13 @@ def _field_fault(field: Any) -> str: """What is wrong with the shape of one field of a draft, or "" if nothing is. - Only ``ranges`` is looked inside for, because it is the only part of a field - anything here reads: `in2lambda.draft.record` compares the lines a command is - quoting against the lines every field was taken from. The value, the layer, whether - it was edited and by whom are written and read back whole, and an edit to any of - them is what a replay catches byte for byte. + ``ranges`` and ``value`` are what is looked for, because they are the parts of a + field anything here reads: `in2lambda.draft.record` compares the lines a command is + quoting against the lines every field was taken from, and + `in2lambda.draft.report.checks` reports a field whose value says nothing. Only + whether there is a value is asked, since the checks look at one as a string or not + at all. The layer, whether it was edited and by whom are written and read back + whole, and an edit to any of them is what a replay catches byte for byte. """ if not isinstance(field, dict): return "is not an object" @@ -44,6 +46,8 @@ def _field_fault(field: Any) -> str: for pair in field["ranges"] ): return f"has ranges {field['ranges']!r} rather than pairs of line numbers" + if "value" not in field: + return "has no value" return "" @@ -54,6 +58,10 @@ def _field_fault(field: Any) -> str: nothing here wrote: there is no command log to replay it from, and inventing an empty one would claim the fields in it came from nowhere. Freezing the source again is the way through, which is what the refusal says. + +A draft `in2lambda validate` has been run on also has a ``report``, which is not required +and not looked into: nothing here reads one back, and the next run of the checks writes +whatever is there over. """ _MARKDOWN = "commonmark_x" @@ -471,6 +479,9 @@ def add(file: str, start_over: bool = False) -> Path: # --start-over is the way to throw them away, and the only one. log: list[Any] = [] fields: dict[str, Any] = {} + # And what the checks found about it, which still holds for the same reason: this + # writes the draft back as it was, so a report of it is a report of what is saved. + report: Any = None # The blocks a draft already here has, which are not always what parsing the # markdown gives: `split block` cuts one in two, and parsing again would undo that # while keeping the log entry saying it happened, leaving the ids the fields were @@ -487,6 +498,7 @@ def add(file: str, start_over: bool = False) -> Path: "invalidates every line range taken from the old draft." ) found, log, fields = existing["blocks"], existing["log"], existing["fields"] + report = existing.get("report") elif frozen_path != source and frozen_path.exists(): raise DraftExists( f"{frozen_path.name} is already there and no {DRAFT} claims it, so it " @@ -514,6 +526,7 @@ def add(file: str, start_over: bool = False) -> Path: "blocks": found, "log": log, "fields": fields, + **({"report": report} if report is not None else {}), }, ) return draft diff --git a/in2lambda/spec/__init__.py b/in2lambda/spec/__init__.py index 10b503b..5b0a1d3 100644 --- a/in2lambda/spec/__init__.py +++ b/in2lambda/spec/__init__.py @@ -324,7 +324,11 @@ def _optional( def _stems(roles: list[Optional[str]]) -> list[Optional[str]]: - """What each question and part is called - ``q1``, ``q1.a`` - in document order.""" + """What each question and part is called - ``q1``, ``q1.p1`` - in document order. + + The same names `in2lambda.draft._next` gives out, so that a draft filled in by a spec + and one filled in by hand hold the same keys, and the checks read either. + """ stems: list[Optional[str]] = [None] * len(roles) questions, parts = 0, 0 for index, role in enumerate(roles): @@ -332,8 +336,8 @@ def _stems(roles: list[Optional[str]]) -> list[Optional[str]]: questions, parts = questions + 1, 0 stems[index] = f"q{questions}" elif role == "part" and questions: - stems[index] = f"q{questions}.{chr(ord('a') + parts)}" parts += 1 + stems[index] = f"q{questions}.p{parts}" return stems diff --git a/tests/fixtures/drafts/README.md b/tests/fixtures/drafts/README.md index 705100c..3efb2d0 100644 --- a/tests/fixtures/drafts/README.md +++ b/tests/fixtures/drafts/README.md @@ -1,18 +1,25 @@ # Drafts built by commands Each folder here is one run: a `source.md` to freeze, the `commands.json` to apply to the draft -of it, and the `expected.json` those commands should leave in the draft's `fields`. The test -freezes the source, applies each command, compares the fields, and then replays the draft from -its log and checks the file is unchanged byte for byte - so a folder covers both what a command -writes and that it can be rebuilt from what it recorded. +of it, the `expected.json` those commands should leave in the draft's `fields`, and the +`report.json` that `in2lambda validate` should then find in it - a folder with no `report.json` +is a draft with nothing wrong with it. The test freezes the source, applies each command, checks +the draft over, compares the fields and the report, and then replays the draft from its log and +checks the file is unchanged byte for byte - so a folder covers both what a command writes and +that it can be rebuilt from what it recorded. -To cover another command, add a folder. `mark_ignore` is the `sources/markdown` document with two -of its blocks marked as nothing to take a question from. `two_questions` is a sheet with a title, a -rubric, two questions with a part each and a separate solutions section, written out by every -command there is: the second question runs into its part with no blank line between them, so the -parser makes one block of the two and `split block` cuts it, and the first question's part is typed -out rather than quoted, because the source writes it with an `(a)` the field should not carry. +To cover another command or another check, add a folder. `mark_ignore` is the `sources/markdown` +document with two of its blocks marked as nothing to take a question from, and the other five +reported as in no field. `two_questions` is a sheet with a title, a rubric, two questions with a +part each and a separate solutions section, written out by every command there is, and it is the +clean one: the second question runs into its part with no blank line between them, so the parser +makes one block of the two and `split block` cuts it, and the first question's part is typed out +rather than quoted, because the source writes it with an `(a)` the field should not carry - which +is why the block it was typed from is marked ignore rather than left unaccounted for. `field_replace` is a sheet the OCR left a brace out of the maths of: one `field replace` puts the brace back and another, with `--regex`, writes a `\tfrac` over the division in the solution, so both fields end up edited while their ranges still name the lines they were quoted from, and the 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. diff --git a/tests/fixtures/drafts/empty_field/commands.json b/tests/fixtures/drafts/empty_field/commands.json new file mode 100644 index 0000000..6415553 --- /dev/null +++ b/tests/fixtures/drafts/empty_field/commands.json @@ -0,0 +1,23 @@ +[ + { + "args": { + "block": "b1" + }, + "by": "tests", + "command": "mark ignore" + }, + { + "args": { + "text": "b2" + }, + "by": "tests", + "command": "question add" + }, + { + "args": { + "text": "s2" + }, + "by": "tests", + "command": "question add" + } +] diff --git a/tests/fixtures/drafts/empty_field/expected.json b/tests/fixtures/drafts/empty_field/expected.json new file mode 100644 index 0000000..d37d178 --- /dev/null +++ b/tests/fixtures/drafts/empty_field/expected.json @@ -0,0 +1,38 @@ +{ + "b1.ignore": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 1, + 1 + ] + ], + "value": true + }, + "q1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 3, + 3 + ] + ], + "value": "Find the pressure at the bottom of the tank." + }, + "q2.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 2, + 2 + ] + ], + "value": "" + } +} diff --git a/tests/fixtures/drafts/empty_field/report.json b/tests/fixtures/drafts/empty_field/report.json new file mode 100644 index 0000000..015677f --- /dev/null +++ b/tests/fixtures/drafts/empty_field/report.json @@ -0,0 +1,13 @@ +[ + { + "check": "empty", + "field": "q2.text", + "message": "q2.text (lines 2-2) is empty.", + "ranges": [ + [ + 2, + 2 + ] + ] + } +] diff --git a/tests/fixtures/drafts/empty_field/source.md b/tests/fixtures/drafts/empty_field/source.md new file mode 100644 index 0000000..0153af9 --- /dev/null +++ b/tests/fixtures/drafts/empty_field/source.md @@ -0,0 +1,3 @@ +# Pressure in a tank + +Find the pressure at the bottom of the tank. diff --git a/tests/fixtures/drafts/field_replace/report.json b/tests/fixtures/drafts/field_replace/report.json new file mode 100644 index 0000000..30d6ccd --- /dev/null +++ b/tests/fixtures/drafts/field_replace/report.json @@ -0,0 +1,24 @@ +[ + { + "check": "uncovered", + "field": "b1", + "message": "b1 (lines 1-1) is in no field and not marked ignore.", + "ranges": [ + [ + 1, + 1 + ] + ] + }, + { + "check": "uncovered", + "field": "b3", + "message": "b3 (lines 6-6) is in no field and not marked ignore.", + "ranges": [ + [ + 6, + 6 + ] + ] + } +] diff --git a/tests/fixtures/drafts/mark_ignore/report.json b/tests/fixtures/drafts/mark_ignore/report.json new file mode 100644 index 0000000..d23d4f2 --- /dev/null +++ b/tests/fixtures/drafts/mark_ignore/report.json @@ -0,0 +1,57 @@ +[ + { + "check": "uncovered", + "field": "b2", + "message": "b2 (lines 3-3) is in no field and not marked ignore.", + "ranges": [ + [ + 3, + 3 + ] + ] + }, + { + "check": "uncovered", + "field": "b3", + "message": "b3 (lines 5-5) is in no field and not marked ignore.", + "ranges": [ + [ + 5, + 5 + ] + ] + }, + { + "check": "uncovered", + "field": "b4", + "message": "b4 (lines 6-6) is in no field and not marked ignore.", + "ranges": [ + [ + 6, + 6 + ] + ] + }, + { + "check": "uncovered", + "field": "b5", + "message": "b5 (lines 8-10) is in no field and not marked ignore.", + "ranges": [ + [ + 8, + 10 + ] + ] + }, + { + "check": "uncovered", + "field": "b6", + "message": "b6 (lines 12-12) is in no field and not marked ignore.", + "ranges": [ + [ + 12, + 12 + ] + ] + } +] diff --git a/tests/fixtures/drafts/part_without_solution/commands.json b/tests/fixtures/drafts/part_without_solution/commands.json new file mode 100644 index 0000000..6c30722 --- /dev/null +++ b/tests/fixtures/drafts/part_without_solution/commands.json @@ -0,0 +1,24 @@ +[ + { + "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" + } +] diff --git a/tests/fixtures/drafts/part_without_solution/expected.json b/tests/fixtures/drafts/part_without_solution/expected.json new file mode 100644 index 0000000..1f0b546 --- /dev/null +++ b/tests/fixtures/drafts/part_without_solution/expected.json @@ -0,0 +1,38 @@ +{ + "b1.ignore": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 1, + 1 + ] + ], + "value": true + }, + "q1.p1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 5, + 5 + ] + ], + "value": "(a) Give the drag coefficient you used." + }, + "q1.text": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 3, + 3 + ] + ], + "value": "A submarine is towed at speed $U$ through still water. Find the drag force on it." + } +} diff --git a/tests/fixtures/drafts/part_without_solution/report.json b/tests/fixtures/drafts/part_without_solution/report.json new file mode 100644 index 0000000..49e68ab --- /dev/null +++ b/tests/fixtures/drafts/part_without_solution/report.json @@ -0,0 +1,13 @@ +[ + { + "check": "no-solution", + "field": "q1.p1", + "message": "q1.p1 (lines 5-5) has no solution: neither q1.p1.solution nor q1.solution is written.", + "ranges": [ + [ + 5, + 5 + ] + ] + } +] diff --git a/tests/fixtures/drafts/part_without_solution/source.md b/tests/fixtures/drafts/part_without_solution/source.md new file mode 100644 index 0000000..6e0c25d --- /dev/null +++ b/tests/fixtures/drafts/part_without_solution/source.md @@ -0,0 +1,5 @@ +# Towing a submarine + +A submarine is towed at speed $U$ through still water. Find the drag force on it. + +(a) Give the drag coefficient you used. diff --git a/tests/fixtures/drafts/two_questions/commands.json b/tests/fixtures/drafts/two_questions/commands.json index 67a89e5..4fd2f4e 100644 --- a/tests/fixtures/drafts/two_questions/commands.json +++ b/tests/fixtures/drafts/two_questions/commands.json @@ -28,6 +28,13 @@ "by": "tests", "command": "part add" }, + { + "args": { + "block": "b4" + }, + "by": "tests", + "command": "mark ignore" + }, { "args": { "at": 12, diff --git a/tests/fixtures/drafts/two_questions/expected.json b/tests/fixtures/drafts/two_questions/expected.json index f81e303..f7373a9 100644 --- a/tests/fixtures/drafts/two_questions/expected.json +++ b/tests/fixtures/drafts/two_questions/expected.json @@ -23,6 +23,18 @@ ], "value": true }, + "b4.ignore": { + "by": "tests", + "edited": false, + "layer": 3, + "ranges": [ + [ + 8, + 8 + ] + ], + "value": true + }, "b6.ignore": { "by": "tests", "edited": false, diff --git a/tests/fixtures/specs/part_part_sol_sol/expected.json b/tests/fixtures/specs/part_part_sol_sol/expected.json index ead90f0..f4a6917 100644 --- a/tests/fixtures/specs/part_part_sol_sol/expected.json +++ b/tests/fixtures/specs/part_part_sol_sol/expected.json @@ -11,7 +11,7 @@ ], "value": true }, - "q1.a.solution": { + "q1.p1.solution": { "by": "tests", "edited": false, "layer": 1, @@ -23,7 +23,7 @@ ], "value": "$W = nRT\\ln(V_1/V_2)$." }, - "q1.a.text": { + "q1.p1.text": { "by": "tests", "edited": false, "layer": 1, @@ -35,7 +35,7 @@ ], "value": "Find the work done on the gas." }, - "q1.b.solution": { + "q1.p2.solution": { "by": "tests", "edited": false, "layer": 1, @@ -47,7 +47,7 @@ ], "value": "$Q = W$, since the internal energy does not change." }, - "q1.b.text": { + "q1.p2.text": { "by": "tests", "edited": false, "layer": 1, @@ -59,7 +59,7 @@ ], "value": "Find the heat rejected." }, - "q1.c.text": { + "q1.p3.text": { "by": "tests", "edited": false, "layer": 1, diff --git a/tests/fixtures/specs/part_sol_part_sol/expected.json b/tests/fixtures/specs/part_sol_part_sol/expected.json index 0a29440..3311b47 100644 --- a/tests/fixtures/specs/part_sol_part_sol/expected.json +++ b/tests/fixtures/specs/part_sol_part_sol/expected.json @@ -11,7 +11,7 @@ ], "value": true }, - "q1.a.text": { + "q1.p1.text": { "by": "tests", "edited": false, "layer": 1, @@ -23,7 +23,7 @@ ], "value": "Find the reaction at A." }, - "q1.b.solution": { + "q1.p2.solution": { "by": "tests", "edited": false, "layer": 1, @@ -35,7 +35,7 @@ ], "value": "$R_B = W/2$, and $R_A$ is the same by symmetry." }, - "q1.b.text": { + "q1.p2.text": { "by": "tests", "edited": false, "layer": 1, diff --git a/tests/fixtures/specs/parts_one_sol/expected.json b/tests/fixtures/specs/parts_one_sol/expected.json index bcd45ff..abed8f3 100644 --- a/tests/fixtures/specs/parts_one_sol/expected.json +++ b/tests/fixtures/specs/parts_one_sol/expected.json @@ -35,7 +35,7 @@ ], "value": true }, - "q1.a.text": { + "q1.p1.text": { "by": "tests", "edited": false, "layer": 1, @@ -47,7 +47,7 @@ ], "value": "Find the thrust." }, - "q1.b.text": { + "q1.p2.text": { "by": "tests", "edited": false, "layer": 1, diff --git a/tests/fixtures/specs/parts_sep_sol/expected.json b/tests/fixtures/specs/parts_sep_sol/expected.json index 02e4286..5af8b67 100644 --- a/tests/fixtures/specs/parts_sep_sol/expected.json +++ b/tests/fixtures/specs/parts_sep_sol/expected.json @@ -11,7 +11,7 @@ ], "value": true }, - "q1.a.solution": { + "q1.p1.solution": { "by": "tests", "edited": false, "layer": 1, @@ -23,7 +23,7 @@ ], "value": "The load is $F = pA$." }, - "q1.a.text": { + "q1.p1.text": { "by": "tests", "edited": false, "layer": 1, @@ -35,7 +35,7 @@ ], "value": "Find the load the large piston carries." }, - "q1.b.solution": { + "q1.p2.solution": { "by": "tests", "edited": false, "layer": 1, @@ -47,7 +47,7 @@ ], "value": "The pressure is $p = F/a$." }, - "q1.b.text": { + "q1.p2.text": { "by": "tests", "edited": false, "layer": 1, diff --git a/tests/test_cli.py b/tests/test_cli.py index 491cb2e..000ed2a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -90,4 +90,5 @@ def test_completing_the_old_form_offers_the_subcommand() -> None: "draft", "source", "spec", + "validate", ] diff --git a/tests/test_draft.py b/tests/test_draft.py index 8975340..60bb9fa 100644 --- a/tests/test_draft.py +++ b/tests/test_draft.py @@ -17,6 +17,7 @@ from conftest import DRAFTS, DRAFTS_DIR import in2lambda.draft +import in2lambda.draft.report from in2lambda.main import cli MARK_IGNORE = DRAFTS_DIR / "mark_ignore" @@ -27,14 +28,23 @@ def _built(folder: Path, tmp_path: Path) -> Path: - """A folder's document, frozen in `tmp_path` with its commands applied to it.""" + """A folder's document, frozen in `tmp_path` with its commands applied and checked.""" shutil.copytree(folder, tmp_path, dirs_exist_ok=True) assert CliRunner().invoke(cli, ["source", "add", "source.md"]).exit_code == 0 for entry in json.loads((folder / "commands.json").read_text()): in2lambda.draft.execute(entry) + # Checked as well as built, so that what a folder's commands leave for the checks to + # find is fixture data like the fields they write are. + in2lambda.draft.report.validate() return tmp_path / "draft.json" +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" + return json.loads(report.read_text()) if report.is_file() else [] + + @pytest.mark.parametrize("folder", DRAFTS, ids=lambda path: path.name) def test_a_draft_built_by_commands_replays_identically( folder: Path, tmp_path: Path, monkeypatch @@ -46,6 +56,7 @@ def test_a_draft_built_by_commands_replays_identically( draft = json.loads(draft_path.read_text()) assert draft["fields"] == json.loads((folder / "expected.json").read_text()) assert draft["log"] == json.loads((folder / "commands.json").read_text()) + assert draft["report"] == _reported(folder) written = draft_path.read_bytes() result = CliRunner().invoke(cli, ["draft", "replay"]) @@ -204,6 +215,7 @@ def test_a_log_entry_that_is_not_a_command_is_refused( ("fields", {"b1.ignore": 5}), ("fields", {"b1.ignore": {"ranges": "s1"}}), ("fields", {"b1.ignore": {"ranges": [[1]]}}), + ("fields", {"b1.ignore": {"ranges": [[1, 1]], "layer": 3}}), ], ids=[ "log", @@ -211,12 +223,13 @@ def test_a_log_entry_that_is_not_a_command_is_refused( "a field that is a number", "ranges that are not a list", "a range that is not a pair", + "a field with no value", ], ) @pytest.mark.parametrize( "arguments", - [["draft", "replay"], ["draft", "mark", "ignore", "b2"]], - ids=["replay", "mark"], + [["draft", "replay"], ["draft", "mark", "ignore", "b2"], ["validate"]], + ids=["replay", "mark", "validate"], ) def test_a_draft_whose_log_or_fields_is_the_wrong_shape_is_refused( field: str, value: Any, arguments: list[str], tmp_path: Path, monkeypatch @@ -290,6 +303,26 @@ def test_lines_another_field_was_taken_from_are_refused( assert draft_path.read_bytes() == built +def test_the_refusal_names_the_lines_that_are_in_the_way( + tmp_path: Path, monkeypatch +) -> None: + """A field edited by hand can be quoted from several ranges, only one of them clashing.""" + monkeypatch.setenv("COLUMNS", "200") + monkeypatch.chdir(tmp_path) + draft_path = _built(MARK_IGNORE, tmp_path) + draft = json.loads(draft_path.read_text()) + # The maths is part of the heading's block as far as this draft is concerned, which + # no command would write but an editor might. + draft["fields"]["b1.ignore"]["ranges"] = [[1, 1], [9, 10]] + draft_path.write_text(json.dumps(draft)) + + result = CliRunner().invoke(cli, ["draft", "question", "add", "--text", "s9:10"]) + + assert result.exit_code != 0 + # Lines 1-1 are free, so naming them would send whoever reads this to the wrong end. + assert "Lines 9-10" in result.output + + @pytest.mark.parametrize( ("arguments", "named"), [ @@ -384,6 +417,61 @@ def test_a_command_says_what_it_wrote(tmp_path: Path, monkeypatch) -> None: assert "regex" not in draft["log"][-1]["args"] +def test_a_draft_edited_into_an_overlap_or_a_gap_is_reported( + tmp_path: Path, monkeypatch +) -> None: + """Neither can be made by a command, so a hand-edited draft is the only way to one.""" + monkeypatch.chdir(tmp_path) + draft_path = _built(TWO_QUESTIONS, tmp_path) + draft = json.loads(draft_path.read_text()) + # Renumbering the second question leaves nothing numbered 2, and giving the first + # question's part the lines the question came from claims those lines twice. + for key in ("q2.text", "q2.p1.text", "q2.solution"): + draft["fields"][key.replace("q2", "q3")] = draft["fields"].pop(key) + draft["fields"]["q1.p1.text"]["ranges"] = [[5, 6]] + draft_path.write_text(json.dumps(draft)) + + result = CliRunner().invoke(cli, ["validate"]) + + assert result.exit_code == 0, result.output + report = json.loads(draft_path.read_text())["report"] + assert [(finding["check"], finding["field"]) for finding in report] == [ + ("overlap", "q1.p1.text"), + ("gap", "q2.text"), + ] + # Both sides of the overlap, so that either field can be looked at without the draft. + assert "q1.text" in report[0]["message"] + assert result.output == f"{report[0]['message']}\n{report[1]['message']}\n" + + +def test_a_clean_draft_is_reported_as_having_nothing_wrong_with_it( + tmp_path: Path, monkeypatch +) -> None: + """A report of nothing is still an answer, and is said rather than printed empty.""" + monkeypatch.chdir(tmp_path) + draft_path = _built(TWO_QUESTIONS, tmp_path) + + result = CliRunner().invoke(cli, ["validate"]) + + assert result.exit_code == 0, result.output + assert result.output == "Nothing to report.\n" + assert json.loads(draft_path.read_text())["report"] == [] + + +def test_a_command_run_after_a_report_leaves_none_behind( + tmp_path: Path, monkeypatch +) -> None: + """A report describes the draft it was run against, and that draft has changed.""" + monkeypatch.chdir(tmp_path) + draft_path = _built(MARK_IGNORE, tmp_path) + assert json.loads(draft_path.read_text())["report"] + + result = CliRunner().invoke(cli, ["draft", "mark", "ignore", "b2"]) + + assert result.exit_code == 0, result.output + assert "report" not in json.loads(draft_path.read_text()) + + def test_the_halves_of_a_split_block_are_blocks_like_any_other( tmp_path: Path, monkeypatch ) -> None: diff --git a/tests/test_spec.py b/tests/test_spec.py index cc3699f..9da62ec 100644 --- a/tests/test_spec.py +++ b/tests/test_spec.py @@ -105,6 +105,30 @@ def test_a_spec_ignoring_a_block_the_draft_has_split_covers_both_halves( assert "is in no field" not in result.output +def test_the_checks_read_the_fields_a_spec_wrote(tmp_path: Path, monkeypatch) -> None: + """A spec names questions and parts as the draft commands do, so `validate` reads both.""" + monkeypatch.chdir(tmp_path) + runner = _frozen(SPECS_DIR / "part_sol_part_sol", tmp_path) + run = runner.invoke(cli, ["spec", "run", "spec.yaml", "--by", "tests"]) + assert run.exit_code == 0, run.output + # The spec answers the second part where it stands and leaves the first unanswered; + # renumbering the second question is the gap, which no command makes. + draft_path = tmp_path / "draft.json" + draft = json.loads(draft_path.read_text()) + for key in ("q2.text", "q2.solution"): + draft["fields"][key.replace("q2", "q3")] = draft["fields"].pop(key) + draft_path.write_text(json.dumps(draft)) + + result = runner.invoke(cli, ["validate"]) + + assert result.exit_code == 0, result.output + report = json.loads(draft_path.read_text())["report"] + assert [(finding["check"], finding["field"]) for finding in report] == [ + ("no-solution", "q1.p1"), + ("gap", "q2.text"), + ] + + def test_a_replay_is_refused_once_the_spec_has_changed( tmp_path: Path, monkeypatch ) -> None: