From ba31a73b395f5a3e9742d0395cab48ccd4523173 Mon Sep 17 00:00:00 2001 From: longvo920 Date: Wed, 29 Jul 2026 22:04:41 +0700 Subject: [PATCH] docs: add an architecture map and fix the docs that drifted from the code The README described features that no longer matched the code and left out the ones added since: the viewer had no mention of Export report (Ctrl+E), the module tree was missing view_model / syntax / review / gitsource, and the add-a-rule note skipped a2l_rules, the shadow step and the beside-a-real-change test that the rule checklist actually requires. docs/architecture.md is the map that was missing entirely: why the core is stdlib-only, what each of the two diff passes decides, why the verdict lives in one function, and what the result dict guarantees downstream. CLAUDE.md stays the rules and now points at it instead of restating it. Corrected against the code rather than assumed: build.ps1 has no -Exe switch (default builds the exe, -Pyz / -PyzOnly add or isolate the zipapp), the smoke command named a script that is not in the repo, and the zipapp is ~110 KB, not the 78-80 KB three files claimed. --- CLAUDE.md | 19 ++- README.md | 17 ++- build.ps1 | 2 +- docs/architecture.md | 274 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 301 insertions(+), 11 deletions(-) create mode 100644 docs/architecture.md diff --git a/CLAUDE.md b/CLAUDE.md index b2376fe..9a350c5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,6 +13,10 @@ Repo `codegen-compare-tool`, local folder `code-review`, package `compare_tool`. Where this file and `~/.claude/CLAUDE.md` disagree, this one wins. +`docs/architecture.md` explains how the pieces fit and why — layering, the two +diff passes, the shared seams, the result-dict contract. Read it before a change +that crosses module boundaries; this file is the rules, that one is the map. + ## 1. Fail-safe is not negotiable **If it cannot be *proven* to be noise, it is a real change.** A rule that is @@ -74,7 +78,7 @@ any dev script take whatever they need; no need to ask first. The exception is **what ships in `compare_tool.pyz`**: the scan, the rules, the diff, the report, the review store and `gitsource` import stdlib only. That -zipapp is 78 KB, needs nothing installed, and is the documented fallback for +zipapp is ~110 KB, needs nothing installed, and is the documented fallback for the machines where antivirus blocks the `.exe` — one third-party import in `scanner.py` and it stops running there, which is a shipped promise broken by an import nobody reviewed. @@ -102,8 +106,12 @@ Two more promises the same machines depend on: Tests pass on layouts that look broken. Every UI change gets looked at: -```bash -QT_QPA_PLATFORM=offscreen python smoke.py # widget.grab().save(png), then read the png +Write a throwaway script under `%TEMP%` that builds the widget, grabs it and +saves a png — then read the png: + +```powershell +$env:QT_QPA_PLATFORM = 'offscreen' +python $env:TEMP\smoke.py # widget.grab().save(png) ``` and, for anything about colour or legibility, a real window on the desktop. @@ -180,6 +188,7 @@ decision; the frozen entry point asks it rather than re-deriving it. ```bash python -m compare_tool --report out.html python -m unittest discover -s tests # unittest, NOT pytest -.\build.ps1 # dist/compare_tool.pyz -.\build.ps1 -Exe # plus the .exe (needs pyinstaller) +.\build.ps1 # dist\compare-tool.exe (needs pyinstaller + PySide6) +.\build.ps1 -Pyz # plus dist\compare_tool.pyz +.\build.ps1 -PyzOnly # zipapp only, no build dependencies ``` diff --git a/README.md b/README.md index 16abc00..e080005 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ scan did find is still printed. | `--exclude PATTERN` | Skip files matching a glob (relative path or bare file name), repeatable. Example: `--exclude compare_report.html` | | `--exit-zero` | Always exit 0 even when real changes exist (report-only mode for pipelines). Compare errors still exit 2 | | `--arxml-only` | Scan only `.arxml`/`.xml`/`.a2l` and write a compact per-type report (default `arxml_update.html`) — always written, even when nothing changed | -| `--review FILE` | Render notes and sign-offs from a review file (`codegen-review.json`, written by the viewer) next to the changes they belong to, with a `Reviewed` badge. Must be named explicitly; no effect with `--arxml-only` | +| `--review FILE` | Render notes and sign-offs from a review file (`codegen-review.json`, written by the viewer) next to the changes they belong to, plus a `Reviewed` badge that hides the changes already signed off. Must be named explicitly — a report must not pick up someone else's sign-off by accident; no effect with `--arxml-only` | | `--qt`, `--viewer` | Open the side-by-side viewer on folders named on the command line, instead of comparing them in the terminal. Needs the `viewer` extra (see below) | Omitting `old_dir`/`new_dir` opens the viewer. `--gui` (the tkinter panel) was removed in 1.1.0. @@ -114,6 +114,8 @@ Reading a scan: `Review mode` adds the note box and a `Review` column in the tree — green when every change in a row is signed off, amber part way, grey when none is. Sign off one change (`Ctrl+R`) or a whole file (`Ctrl+Shift+R`); the notes travel into the exported report. +`Export report…` (`Ctrl+E`) writes the same self-contained HTML report the CLI writes, with the review notes folded in. It is always built from the **complete scan**, never from what is on screen — a category you collapsed in the tree still appears in the file with its real verdict. + Full walkthrough is built into the app — `Help` → `User guide` (`F1`), which works offline. Standalone `.exe` (no Python needed): see [Single-file build](#single-file-build). ## What counts as noise @@ -166,7 +168,6 @@ A file whose XML fails to parse is skipped from this summary (its text diff stil Files are grouped by **Simulink model** using the Embedded Coder AUTOSAR naming convention (`X.c`, `X.h`, `X.arxml`, `Rte_X.h`, the modular ARXML set, …). Files that match no model land in a final **Shared / other** group. - ## HTML report Self-contained file, one per compare: badge toggles, folder tree, filter box, collapsible diffs per file. Opens `Unimportant` hidden and `Modified` expanded, so it opens on what matters. @@ -189,7 +190,7 @@ See [azure-pipelines.yml](azure-pipelines.yml) for a working example (OLD checke ```powershell .\build.ps1 # dist\compare-tool.exe - one file, nothing to install on the target -.\build.ps1 -Pyz # also dist\compare_tool.pyz (~80 KB) for machines that have Python 3.8+ +.\build.ps1 -Pyz # also dist\compare_tool.pyz (~110 KB) for machines that have Python 3.8+ .\build.ps1 -PyzOnly # zipapp only (building it needs no PyInstaller / PySide6) ``` @@ -203,7 +204,7 @@ See [azure-pipelines.yml](azure-pipelines.yml) for a working example (OLD checke Built as a **console** application so terminal runs keep their exit code (`1` = real changes, `2` = compare incomplete) for the CI gate. The viewer hides the console window at runtime — you'll see a brief flash on double-click. A crash un-hides the console so the error is visible. -- **`.pyz` (zipapp, stdlib)**: `python compare_tool.pyz [flags]`. Prefer it when Python is available — ~80 KB, no build dependencies, not flagged by antivirus. The CLI works anywhere; the viewer additionally needs PySide6 on that machine (without it, the tool says so instead of opening). +- **`.pyz` (zipapp, stdlib)**: `python compare_tool.pyz [flags]`. Prefer it when Python is available — ~110 KB, no build dependencies, not flagged by antivirus. The CLI works anywhere; the viewer additionally needs PySide6 on that machine (without it, the tool says so instead of opening). - **`.exe` (PyInstaller onefile, ~47 MB)**: no Python needed on the target. Building needs `pyinstaller` and `PySide6` on the dev machine (`build.ps1` installs them), and the binary only runs on the OS it was built on. PyInstaller executables are sometimes blocked by antivirus or AppLocker — fall back to the `.pyz` there. Every CLI flag behaves identically in the packaged builds. `build/` and `dist/` are already in `.gitignore`. @@ -226,10 +227,16 @@ compare_tool/ ├── c_rules.py # C/H rules: strip comments, tokenize, detect renames, extract RTE access points ├── arxml_rules.py # ARXML rules: UUID, ADMIN-DATA, DATE, comments + extract port interfaces, SWCs (ports/runnables/events) ├── a2l_rules.py # A2L rules: strip C-style comments + extract CHARACTERISTIC/MEASUREMENT +├── view_model.py # renderer-agnostic view model (paint mode, intra-line span, row alignment) shared by the report and the viewer +├── syntax.py # line-at-a-time C / XML token spans, Qt-free so it ships in the .pyz +├── review.py # reviewer notes and sign-offs, keyed by change content so they survive a rescan +├── gitsource.py # read-only `git archive` of a commit into a temp folder, so a commit can be the OLD side └── report.py # self-contained HTML report (badge toggles, model overview, grouping, filter, collapsible diffs) ``` -To add a rule: write the strip function in `c_rules.py` / `arxml_rules.py`, then register it in the shadow builder and `_build_variants` in `diff_engine.py`. +[docs/architecture.md](docs/architecture.md) covers how these fit together and why: the two diff passes, where a verdict is decided, the shared seams and the result-dict contract. Anything both renderers need lives in `view_model.py` — reimplementing a mapping inline lets the HTML report and the viewer drift apart about what changed. + +To add a rule: write the strip function in `c_rules.py` / `arxml_rules.py` / `a2l_rules.py`, join it into that ruleset's shadow, register one labelled variant in `_build_variants` in `diff_engine.py`, and add both tests — the pattern alone is noise, and the same pattern *beside* a real change still reports the real change. Issues and pull requests are welcome. Please keep the **compare core stdlib-only** — it has to run on locked-down build servers, so PySide6 stays confined to `compare_tool/qtviewer/` and is imported only when the viewer opens — and add a test under `tests/` for any new rule. diff --git a/build.ps1 b/build.ps1 index 4e34897..1101198 100644 --- a/build.ps1 +++ b/build.ps1 @@ -10,7 +10,7 @@ # and the exit code; the viewer hides the console # window at runtime (see packaging\entry.py). # -# dist\compare_tool.pyz optional tiny zipapp (~80 KB) for machines that +# dist\compare_tool.pyz optional tiny zipapp (~110 KB) for machines that # already have Python 3.8+. Stdlib only, so the CLI # works anywhere; the viewer additionally needs # PySide6 installed on that machine. diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..64d72e6 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,274 @@ +# Architecture + +How the compare tool is put together, and why. For what it *does* — flags, +noise rules, screenshots — read the [README](../README.md) first; this document +is for someone about to change the code. + +## The claim the design has to protect + +The product is one sentence: **"you can ignore what I hid."** + +Regenerating a Simulink model rewrites timestamps, UUIDs, comment banners and +auto-generated identifiers even when the behaviour is identical. The tool +classifies every difference so a reviewer is not drowned in that churn. The +moment it hides one real change, a reviewer stops trusting the filter and the +tool is worse than `diff`. + +Every structural decision below follows from that: **anything not *proven* to +be noise is a real change**, and anything that could not be compared at all is +louder still. + +## Layers + +```mermaid +flowchart TD + subgraph front[Front ends] + CLI[main.py
CLI + exit code] + QT[qtviewer/
PySide6 viewer] + end + subgraph core[Compare core — stdlib only] + SC[scanner.py
walk + pair + fold] + DE[diff_engine.py
two-pass diff + verdict] + RULES[c_rules · arxml_rules · a2l_rules
strip, tokenize, extract] + end + subgraph shared[Shared seams] + VM[view_model.py
mode_of · char_span · aligned_rows] + RV[review.py
notes keyed by content] + SY[syntax.py
token spans, Qt-free] + end + RP[report.py
self-contained HTML] + GS[gitsource.py
commit → temp folder] + + CLI --> SC + QT --> SC + GS --> QT + SC --> DE + DE --> RULES + CLI --> RP + QT --> RP + RP --> VM + QT --> VM + QT --> SY + RP --> RV + QT --> RV +``` + +Two rules hold this shape: + +**The core imports nothing but the standard library.** `scanner`, `diff_engine`, +the three rule modules, `report`, `review`, `view_model`, `syntax` and +`gitsource` are what ships in `compare_tool.pyz` — ~110 KB, no install, the +documented fallback for machines where antivirus blocks the `.exe`. One +third-party import in `scanner.py` and the zipapp stops running there. PySide6 +lives only under `compare_tool/qtviewer/` and is imported lazily, when the +viewer opens, so the test suite runs headless. + +**Arrows only point down.** The core never imports a front end. `syntax.py` +says *what* a stretch of text is and never what colour it gets, so the Qt layer +and any second surface can reuse it without the mapping being written twice. + +## Data flow of one compare + +```mermaid +sequenceDiagram + participant F as Front end + participant S as scanner.scan + participant D as diff_engine.compare_pair + participant R as Renderer + + F->>S: old_root, new_root, exclude/include + S->>S: list_files() both sides, capture listing errors + loop each relative path + alt in both trees + S->>D: old_text, new_text, rel + D->>D: pass 1 — raw line diff + D->>D: pass 2 — shadow diff (+ rename map) + D->>D: label each raw hunk, then _status_of + D-->>S: {status, hunks, renames, notes} + else one side only + S->>S: added / deleted + semantic extras + else unreadable + S->>S: status 'error' — loud, never silent + end + end + S-->>F: {rel_path: result} + F->>R: the SAME dict, unfiltered +``` + +### The two passes + +`compare_pair` diffs the files twice. + +- **Pass 2 decides the truth.** Each side is reduced to a *shadow*: comments + stripped, whitespace collapsed, UUIDs and dates and version stamps removed, + and for C a verified 1-to-1 rename map applied. Whatever still differs + between the two shadows is a real change. The rename map is best-effort and + then *checked* — it is applied to the old shadow and re-diffed, and any line + it does not fully explain stays real. +- **Pass 1 decides what you see.** The raw line diff keeps every textual + difference, so the viewer can show the churn instead of pretending the files + were identical. A raw hunk that intersects no real hunk is ignorable, and is + *labelled* by `_build_variants`: a list of shadows each with exactly **one** + rule applied. The first variant under which the hunk's two slices are equal + names it (`comment`, `uuid`, `timestamp`, `sw-version`, `rename`, + `whitespace`). A hunk that no single rule explains is `mixed` — still + ignorable, but honest that more than one rule combined to explain it. + +This is why the noise-rule checklist is what it is: a new rule has to be +text-based and preserve line count (or the two passes stop lining up), joined +into the ruleset's shadow, *and* given a labelled variant. Miss the variant and +the rule silently becomes `mixed`. + +### The verdict lives in one function + +`diff_engine._status_of` is the only place a status is decided: + +| Status | Meaning | Foldable | +|---|---|---| +| `identical` | no difference at all | — | +| `comment-only` | every labelled hunk is `comment` | yes | +| `ignorable-only` | noise, but not comments alone | yes | +| `real-change` | at least one hunk survived pass 2 | **no** | +| `added` / `deleted` | present on one side only | **no** | +| `error` | could not be listed, read or compared | **no** | + +`comment-only` is deliberately separate from `ignorable-only`: "the banner +moved" triages differently from "an identifier was renamed". A file mixing +comments *with* other noise stays `ignorable-only` — the narrower claim has to +be exact. + +`scanner.FOLDABLE` names the only two statuses a UI toggle may collapse. Real +changes, one-sided files and errors are absent from that tuple **by +construction**, so no caller mistake can hide one. + +### Folding is a pure function, not a rescan + +`scanner.apply_fold` re-judges an already scanned tree under different rules +with no disk access: the hunks already say what kind each difference is, so +re-reading every file to learn the same thing is pure waste. It copies rather +than mutates, so the rules can be toggled back and forth. The viewer keeps the +untouched scan in `MainWindow._raw_results` and folds into `self.results` for +display. + +## The result dict is the contract + +Everything downstream — CLI summary, HTML report, viewer tree, review store — +consumes one dict per compared path: + +```python +{ + 'status': 'real-change', + 'hunks': [{'kind': 'real', 'old_range': [12, 15], 'new_range': [12, 14]}, + {'kind': 'moved', 'old_range': [40, 60], 'new_range': [40, 40], + 'moved_to': 91}], + 'renames': {'rtb_AND_c4nxjoom3d': 'rtb_AND_j2kqp1wxab'}, + 'notes': ['line-endings'], + 'binary': False, + # semantic extras, only on real changes and one-sided files: + 'ifaces': ..., 'swc': ..., 'rte': ..., 'a2l': ..., +} +``` + +Ranges are 0-based, end-exclusive, into the **raw** lines of each side. +`kind` is one of `real`, `moved`, `comment`, `rename`, `uuid`, `timestamp`, +`sw-version`, `whitespace`, `mixed`. + +The semantic extras are computed only where they can matter: a shadow-equal +file has the same content, so it cannot have moved the AUTOSAR surface. + +## Shared seams + +Any fact two renderers need lives in one module they both import. The failure +mode this prevents is quiet: two copies of a four-line mapping agree perfectly +until someone adds a new kind to one of them. + +- **`view_model.mode_of`** — hunk kind → paint mode (`real`, `moved`, + `comment`, `minor`). The HTML report and the Qt panes cannot disagree about + how a kind is coloured or whether it can be hidden. +- **`view_model.char_span`** — the intra-line highlight as plain character + offsets. The report wraps them in a ``; the viewer applies a + `QTextCharFormat` over the same numbers. +- **`view_model.aligned_rows` / `collapse_rows`** — whole-file two-pane + alignment, and folding a run of noise rows into one `⋯ N uuid lines hidden` + placeholder that reads as context on both sides so the panes stay in scroll + lockstep. The count is always stated: this hides noise, it does not drop it. +- **`review.py`** — notes and sign-offs keyed by a hash of the change's own + text, not by line number, so an unrelated edit elsewhere in the file does not + detach them on the next scan. + +## Front ends + +`main.viewer_requested(argv)` owns the "which front end does this argv want" +decision, and answers it **without running the compare** — the frozen entry +point calls it to hide the console window before Qt starts. The rule: the +terminal compare runs only when both folders are named on the command line; +everything else opens the viewer. + +### CLI + +`run_compare` deletes any leftover report *before* scanning — if this run dies, +a stale file from the previous one must not pass for its result. A report path +that cannot be written raises `ReportWriteError` carrying the scan it could not +write, so the terminal still prints what was found, and the run exits `2`: + +| Code | Meaning | +|---|---| +| 0 | No real change | +| 1 | Real changes found (the CI gate) | +| 2 | Compare INCOMPLETE — a path could not be listed, read or compared, or the report could not be written | + +The exit code is a contract with somebody's pipeline. `--exit-zero` suppresses +`1`, never `2` — an incomplete compare must never look green. + +### Viewer + +The scan walks the disk, so it runs on a `QThread` (`qtviewer/worker.py`) and +results cross back through signals only. Widgets stay dumb: they walk a model +and paint it. `tree.py`, `summary_model.py` and `compare_tool/resources.py` +have no PySide6 import at all, so their tests run on a box with no Qt. + +`Git compare…` is not a second compare mode. `gitsource.py` runs `git archive` +— read-only, touching neither HEAD, the index nor the working tree — to lay one +commit down in a temp folder, and everything downstream sees two directories as +usual. That matters because the folder being reviewed is normally the one the +engineer is still editing. + +## Decisions worth knowing before you change something + +**The record is never the filtered view.** `MainWindow._export_report` builds +from `_raw_results`, not from what is on screen. A category the reviewer +collapsed must still appear in the exported file with its real verdict — +otherwise an export could show a file as Identical when it is not. Same for the +quick-changes rollup. + +**The HTML report is self-contained.** CSS and JS inline, no CDN, nothing +fetched when the file is opened. It gets mailed around and opened on machines +with no internet; a report that renders blank there is worse than no report. + +**Cosmetic failures degrade, the compare does not.** A missing icon leaves a +button with its text label (`resources.py` getters return `None` and callers +cope); no PySide6 gives a plain sentence about the `viewer` extra, not a +traceback; Windows legacy codepages are handled with +`stream.reconfigure(errors='replace')` so a print can never kill a run. The +compare itself is the exception — a scan or render failure is loud, never an +empty, clean-looking result. + +**The `.exe` is a console build on purpose.** A windowed build makes the shell +stop waiting and throws the exit code away, which breaks the CI gate. So the +console *window* is hidden at runtime instead (`packaging/entry.py`), and +un-hidden on a crash. + +**Python 3.8 is a shipped promise.** No `match`, no `X | Y` at runtime; +`list[str]` in an annotation needs `from __future__ import annotations`. CI runs +3.8 and 3.11 on Linux and Windows, so a 3.10-ism passes locally and fails there. + +## Where to change what + +| Change | Touch | +|---|---| +| New noise rule | the rule module's strip function → that ruleset's shadow → one labelled variant in `_build_variants` → two tests (alone it is noise; beside a real change it stays real) | +| New file type | `RULES` in `diff_engine.py`, a `*_rules.py` module, shadow + variants | +| New semantic extraction | `*_rules.py` extractor, wire into `scanner.compare_file` and `_single_info`, then a `summarize_*` rollup | +| Anything both renderers show | `view_model.py` — never inline in one of them | +| New verdict | `diff_engine._status_of`, and decide explicitly whether it belongs in `scanner.FOLDABLE` (default: no) | +| Viewer layout or colour | render it and look at it (`widget.grab().save(png)` under `QT_QPA_PLATFORM=offscreen`), then a real window |