alunduil / alunduil/zfs-replicate

The CLI pipeline groups snapshots once, separates planning from execution, and dispatches actions by table

Abierto
#620 0 comentarios 0 reacciones 0 asignados Ver en GitHub
enhancement python
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

Abrir la 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

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.