alunduil / alunduil/zfs-replicate
The CLI pipeline groups snapshots once, separates planning from execution, and dispatches actions by table
- Lenguaje dominante
- Python
- Estrellas
- 24
- Forks
- 6
- Merge medio
- 3 h 11 min
- PR fusionados (30 d)
- 49
Descripción
## Summary
Apply five refactorings from Fowler's catalogue to `cli/main.py`, `task/execute.py`, and `task/report.py`: Extract Function on a grouping idiom written seven times, Split Phase on `main()`, Change Function Declaration then table dispatch for the `Action` chain, Introduce Parameter Object on the bundle threaded through the send path, and Rename Variable on the abbreviations. Behaviour does not change.
## Current behaviour
**The same grouping idiom appears seven times, in two shapes.** Three build a dict: `cli/main.py:107-109` and `cli/main.py:110-112`, identical apart from variable names, and `task/execute.py:28-31`, keyed on `.action` rather than `.filesystem`. Four build a list of pairs: `cli/main.py:121-123`, `task/report.py:16-18`, `task/report.py:32`, and `task/report.py:48-52`, the last with an extra `if snapshot is not None` filter. `task/report.py:32` keys on the module-level `_action` (`task/report.py:83-84`), which exists only because a lambda in that position does not type; its comment says as much.
**Every grouping site assumes contiguous input.** `itertools.groupby` groups only adjacent equal keys, so all seven depend on `zfs list -r` emitting each filesystem's snapshots together. `_add_previous` (`snapshot/list.py:73-75`) resets `previous` at a filesystem boundary, which shows the assumption is deliberate, but no grouping site records it. A shared helper would bake it in silently.
**`main()` runs the whole pipeline inline.** `cli/main.py:89-131` performs SSH command construction, local listing, remote data set creation, remote listing, three groupings, task generation, reporting, and execution in one body. The seam is already visible: `--dry-run` is tested twice, as `if dry_run:` at `cli/main.py:117` and `if not dry_run:` at `cli/main.py:120`, because planning and execution interleave rather than sequence.
**The action chain has no fallthrough.** `task/execute.py:33-46` dispatches on `Action` through `if`/`elif` with no `else`. `Action` (`task/type.py:10-15`) declares exactly `CREATE`, `DESTROY`, and `SEND`, and all three are handled, so there is no defect today. A fourth member added later would be dropped silently instead of raising. `_create` and `_destroy` take `(tasks, ssh_command)` while `_send` takes four more keyword-only arguments, so Change Function Declaration to a uniform signature is a prerequisite for any table.
**Four internal signatures repeat one `PLR0913` suppression.** `snapshot/send.py:35`, `snapshot/send.py:63`, `task/execute.py:15`, and `task/execute.py:69` all read `# noqa: PLR0913 -- carries the full replication call surface`, and all four thread the same `ssh_command` / `compression` / `send_options` / `receive_options` bundle through unchanged. The comment names the concept it suppresses. `cli/main.py:72` carries the same rule for a different reason: Click injects each command-line option as its own parameter.
**`main()` abbreviates what the layer below spells out.** `l_snaps`, `r_snaps`, and `r_filesystem` (`cli/main.py:93-112`) and `a_tasks` (`task/execute.py:33`) name the concepts that `generate()` already declares as `local_snapshots` and `remote_snapshots` (`task/generate.py:21-22`).
## Motivation
Found reviewing #617 against Fowler's catalogue, and re-checked against HEAD while working #505.
Each of these is a named move with a mechanical diff and no behaviour change, so the existing suite is the check. The parameter object is the one with a countable outcome: it deletes four suppressions rather than explaining the same clump in four places.
## Approach and alternatives
The moves, ordered so each unblocks the next:
1. Introduce Parameter Object on the replication bundle. Independent of the rest.
2. Extract Function on the grouping idiom, taking a key function.
3. Change Function Declaration on `_create`, `_destroy`, and `_send`, then replace the `if`/`elif` with a table that raises on an unhandled `Action`.
4. Split Phase on `main()`, reducing `--dry-run` to one branch.
5. Rename Variable on the abbreviations, last, so it lands on the split-out phases rather than being undone by them.
Set aside:
- **Replace Conditional with Polymorphism** on the `Action` chain. Three branches on a closed enum, and polymorphism forces a common signature onto `_create` and `_destroy` that neither needs. The table closes the fallthrough hole at less cost.
- **Introduce Parameter Object on `main()`**. Click supplies flat keyword arguments, and `cli/options.py` already collapses the flag groups into `send.Options` and `receive.Options`.
- **One helper for all seven grouping sites.** The dict and list-of-pairs shapes differ in return type and `task/report.py:48-52` filters, so a single signature spanning both may cost more than two small helpers. Settle it with the diff in front of you.
## Scope
zfs/replicate/cli/main.py, zfs/replicate/task/execute.py, zfs/replicate/task/report.py, zfs/replicate/snapshot/send.py
## Out of scope
- `cli/main.py:72`'s own `PLR0913`. Its justification differs and it stays.
- Moving the configuration onto `Task`. `generate()` and `report()` need none of it, `--dry-run` builds tasks without opening a connection, and `Task` is a dataclass whose structural equality the tests compare.
- Replacing the adjacency assumption with an explicit sort. Recording it is in scope; changing it is a behaviour change.
## Acceptance criteria
- [ ] The grouping idiom is written once per shape, and the helper records the adjacency it depends on
- [ ] `task/report.py`'s `_action` is absorbed by the helper's key parameter or its reason for existing is stated
- [ ] `main()` reads as named phases and tests `dry_run` once
- [ ] An `Action` member with no handler raises rather than doing nothing
- [ ] `snapshot/send.py` and `task/execute.py` carry no `PLR0913` suppression
- [ ] `main()` and `execute()` use the names `generate()` already declares
- [ ] The commit and pull request name each move applied and each entry declined
- [ ] The existing tests pass without modification, since none of this changes behaviour
## Related issues
#513 is open and rewrites this exact path, adding `_run` and binding the bundle with `functools.partial` to feed `schedule.dispatch`. Whichever lands second takes the conflicts; if #513 lands first, its `_run` and that partial application join the scope here.
#605 would raise confidence that behaviour really is preserved, since it verifies replication against real pools.
#622 covers the comment cleanup in this area, `task/report.py:82` included.
#602 asked for the same value object against #513's branch and is closed as a duplicate of this issue.
#475 and #501 are both closed, so the annotation conflict an earlier revision of this issue warned about is gone: #695 landed the PEP 585 rewrite.
Guía de contribución
Línea de trabajo
Start by reading cli/main.py:89-131 and the referenced paths in task/execute.py, task/report.py, and snapshot/send.py, then run the existing test suite to establish the behavior baseline. Track the five proposed refactorings and the related #513 interaction before changing scope. Done means the acceptance criteria hold, including one dry-run branch, guarded Action dispatch, no specified PLR0913 suppressions, preserved tests, and named phases.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- python
- Área
- cli
- Tipo de issue
- Refactorización
- Dificultad
- 5/5
- Tiempo estimado
- Más de una semana
- Estado de actividad
- Tranquilo
- Claridad
- Bien especificado
- Aptitud para principiantes
- 35/100