|
|
@@ -0,0 +1,100 @@
|
|
|
+# Go readability
|
|
|
+
|
|
|
+Branch: `go-readability` → `main`
|
|
|
+
|
|
|
+PR 10 of the review fixes plan (`docs/review-fixes-plan.md`), a readability PR: no behaviour change, no version change. It was done after PR 11; the two touch different files.
|
|
|
+
|
|
|
+## What changed
|
|
|
+
|
|
|
+Four commits, one per area:
|
|
|
+
|
|
|
+1. **`Service.state` split into named helpers (R3, R4).**
|
|
|
+ - **The helpers:** `state()` in `internal/app/app.go` used `p`, `v`, `q`, `f`, `m` and `st` across a 58-line function. It now builds the `State` from four helpers:
|
|
|
+ - `limits()`;
|
|
|
+ - `missingSources(proj)`, the pool files export could not copy;
|
|
|
+ - `padState(proj, slot, missing)`;
|
|
|
+ - `sequenceStates(proj)`.
|
|
|
+
|
|
|
+ They use names such as `proj`, `view` and `seq`.
|
|
|
+ - **The rename:** `Service.edit` becomes `apply`.
|
|
|
+2. **Clearer names and notes in `xpj` and `project` (R4, R6).**
|
|
|
+ - **Renames, with the comments that mention them:**
|
|
|
+
|
|
|
+ | Was | Now | What it does |
|
|
|
+ | --- | --- | --- |
|
|
|
+ | `xpj.wrap` | `wrapErr` | Prefixes JSON errors with `xpj:`. |
|
|
|
+ | `project.wrap` | `fromFile` | Builds a `Project` from a decoded file. |
|
|
|
+ | `project.edit` (type) | `setter` | Applies several `Set`s and keeps the first error. |
|
|
|
+
|
|
|
+ Together with `apply`, no two things share a name any more.
|
|
|
+ - **`uniqueFile`:** it kept taken file names and sample names in one map, told apart by a `"\x00"` suffix. Now it uses two maps, `takenFiles` and `takenNames`, and its comment says both kinds of name are kept unique.
|
|
|
+ - **The BPM check:** `!(bpm >= MinBPM && bpm <= MaxBPM)` now has a comment saying it is written that way so NaN, for which every comparison is false, is refused.
|
|
|
+3. **`main()` reads as a list of steps (R5).**
|
|
|
+ - `main()` in `main.go` goes from 96 lines to 12. It calls four new functions, each keeping the comment that was there:
|
|
|
+ - `newApp`: the application options and the panic handler;
|
|
|
+ - `newWindow`: the hidden window, sized and shown once the screens are known;
|
|
|
+ - `guardClose`: the unsaved-changes guard;
|
|
|
+ - `handleFileDrops`: the file-drop handler.
|
|
|
+ - The steps run in the same order as before.
|
|
|
+4. **The CLI's flags each have a named parse function (R5).**
|
|
|
+ - **Parsing:** `init()` in `cmd/mpckit-cli/main.go` was 104 lines of closures. Each flag now has its own function: `parsePadOp`, `parseTuneOp`, `parseMuteOp`, `parseClearOp`, `parseCopyOp`, `parseMoveOp`, `parseSwapOp`, `parseSeqOp` and `parseRmseqOp`. `init()` registers them from a nine-row table.
|
|
|
+ - **`main()`:** it was 62 lines, over the plan's 60. It now hands opening the project to `openProject` and exporting to `write`.
|
|
|
+ - **Unchanged:** flag names, usage text, error messages and output.
|
|
|
+
|
|
|
+**Longest functions** in `main.go`, `cmd/mpckit-cli/main.go` and `internal/app/app.go`:
|
|
|
+
|
|
|
+| | Longest | Next |
|
|
|
+| --- | --- | --- |
|
|
|
+| Before (`main`) | CLI `init` 104 lines | `main()` 96, CLI `main` 62, `state` 58 |
|
|
|
+| Now | 28 lines: `padState`, `parseSeqOp` and the CLI `main` | `newApp` and `openProject`, 25 each |
|
|
|
+
|
|
|
+## Notes
|
|
|
+
|
|
|
+- **Name choices.** I used the plan's names: `wrapErr`, `fromFile`, `apply` and `setter`.
|
|
|
+ - **New in the CLI:** a function type for one flag's change to the project, which I named `opFunc`, beside the existing `op` and `opFlag`. I avoided calling it `edit`, which this PR removes as a name elsewhere.
|
|
|
+ - **The two CLI helpers:** `openProject` and `write`.
|
|
|
+- **Two small changes in how the CLI's parse functions are written, with the same result:**
|
|
|
+ - `-clear` and `-rmseq` used to return a function together with the error. They now return nil on error. `flag` never stored the function when there was an error, so nothing a user sees changes.
|
|
|
+ - `-tune`'s long out-of-range message is now split across two lines of source; the text is the same.
|
|
|
+- **Not changed:** what each function does, the order of `main()`'s steps, and the tests. `load_test.go` changes only for the `fromFile` rename.
|
|
|
+
|
|
|
+## Testing
|
|
|
+
|
|
|
+**On the server, every command with `GOTOOLCHAIN=go1.27.4`:**
|
|
|
+- `go test ./...` passes. No test was removed or loosened; one renamed call and comment in `load_test.go`.
|
|
|
+- `go vet ./...` (also with `GOOS=windows`) and `staticcheck ./...` are clean.
|
|
|
+- `govulncheck ./...`: "No vulnerabilities found."
|
|
|
+- In `frontend`: `npm ci`, `npm run check` (0 errors, 0 warnings), `npm run build` and `npm run format:check` pass. The frontend is unchanged.
|
|
|
+- `wails3 build` builds and the app starts under Xvfb.
|
|
|
+
|
|
|
+**"No behaviour change", checked against `main` before the first commit:**
|
|
|
+- **Exports are byte-identical.** I ran `go run ./cmd/mpckit-cli -load testdata/device/DemoFile.xpj -out <dir>` on `main` and after the last commit. `cmp` finds the two `DemoFile.xpj` identical, and `diff -r` finds the data folders identical.
|
|
|
+- **`go run ./cmd/mpckit-cli -h`** prints exactly the same text.
|
|
|
+- **22 CLI runs give the same results.** Two runs use every flag (one exports to a folder, one to a zip). The rest are 15 bad flag values, an existing export with and without `-force`, a stray argument, both `-out` and `-zip`, and no `-name`. For each run I compared stdout, stderr, exit code, the exported files, and the zip's entries and their contents (the zip itself stores its write time).
|
|
|
+- **The State the window gets is byte-identical.** A scratch test, not committed, wrote the full `State` JSON after loading DemoFile, ten edits, two refused edits, loading a copy with a missing WAV, and the refused export.
|
|
|
+- **No bound method changed:** `TestBoundMethods` passes untouched, and `frontend/bindings` did not change.
|
|
|
+
|
|
|
+**On the laptop (PowerShell, in the repo).** This changes the window set-up in `main.go`, so watch the window size, the close prompt and file drop in particular:
|
|
|
+1. `git fetch; git checkout go-readability`
|
|
|
+2. `go test ./...`: all `ok`.
|
|
|
+3. `go run ./cmd/mpckit-cli -h`: the same usage text as before.
|
|
|
+4. `wails3 dev`. The window opens at the usual size, centred, with nothing scrolling.
|
|
|
+5. **About:** open About and check the version and links. Close it with Escape.
|
|
|
+6. **Pads tab:** load `testdata\device\DemoFile.xpj`.
|
|
|
+ - Click pads to hear them. The pad panel shows names and files.
|
|
|
+ - Change a tune and a mute group.
|
|
|
+ - Drop a WAV onto an empty pad: it is assigned. Drop a non-WAV file: "Only WAV files can be assigned to a pad." shows.
|
|
|
+ - Move/Copy to pad: the picker opens. Drag A1 onto A5: the notes warning shows; Cancel.
|
|
|
+7. **Sequences tab:**
|
|
|
+ - Select Intro and Sequence 02.
|
|
|
+ - Change a name and a BPM.
|
|
|
+ - Set Sequence 02 to 1 bar: the "Shorten…" dialog shows; Cancel.
|
|
|
+ - Add a sequence in B5, then remove it: the "Remove…" dialog shows.
|
|
|
+8. **Export:**
|
|
|
+ - Export to a new folder.
|
|
|
+ - Export there again and Cancel at "Replace existing files?".
|
|
|
+ - Export a zip.
|
|
|
+9. **Missing file:** load a copy of DemoFile with one WAV removed. The missing badge and the footer warning show, and export is disabled.
|
|
|
+10. **New and close:**
|
|
|
+ - New, with unsaved changes: the confirm dialog shows.
|
|
|
+ - Make a change, then close the window: "Close without exporting?" shows. Cancel keeps the app open; confirming closes it.
|
