alunduil / alunduil/zfs-replicate

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

Đang mở
#620 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
enhancement python
Ngôn ngữ chính
Python
Star
24
Fork
6
Merge trung bình
3 giờ 11 phút
Pull request đã merge (30 ngày)
49

Mô tả

## 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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
cli
Loại issue
Tái cấu trúc
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
35/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.