Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(commands): decouple logic from option structs for check, prune, repair, key, and restore #317

Merged
merged 9 commits into from
Oct 9, 2024

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Oct 6, 2024

I saw in #224 that CheckOptions::run() has been moved to type CheckResultsCollector. In other commands we actually have functions, not associated to any type, that take their options as parameters. I applied this here to check, prune, repair, key, and restore as well. Because I think it reduces coupling and increases testability.

The idea behind having these standalone functions is, that check, prune, repair, key, and restore are not run on their options (as a method of e.g. CheckOptions), but rather take parameters, where one is the e.g. CheckOption itself.

In principle: methods that implement commands don't operate on their own options and have side effects on other types - options are passed into functions as parameters.

Furthermore, a check() on CheckOptions sounds if it validates these options itself, rather than being run on an external type. I think from that POV it also makes sense to have such freestanding functions as entry point to our commands in rustic_core.

@simonsan simonsan added A-testing Area: Testing and coverage M-breaking Meta: Breaking change A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience C-refactor Category: Refactoring of already existing code labels Oct 6, 2024
Copy link

codecov bot commented Oct 6, 2024

@simonsan simonsan marked this pull request as draft October 6, 2024 01:39
@aawsome
Copy link
Member

aawsome commented Oct 6, 2024

I agree to this refactor. Most changes are actually non-breaking as they are all offered to the users via the Repository API. I would suggest having this as a non-breaking change, see my comment.

Besides this, we should either postpone the topic or (preferably) merge it fast. Leaving it for some time just increases the cost of this... ;-)

@simonsan simonsan marked this pull request as ready for review October 6, 2024 20:22
@simonsan simonsan changed the title refactor!: decouple logic from option structs for check, prune, repair refactor: decouple logic from option structs for check, prune, repair, key, and restore Oct 6, 2024
@simonsan simonsan removed the M-breaking Meta: Breaking change label Oct 6, 2024
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan force-pushed the refactor/decouple_types branch from 2c73041 to fc3d65e Compare October 7, 2024 00:41
@simonsan
Copy link
Contributor Author

simonsan commented Oct 7, 2024

It's true, all the changes are non-breaking now. I added the deprecation notes as well.

I made single commits for each command being decoupled and force pushed, so it's easier to review.

@simonsan simonsan changed the title refactor: decouple logic from option structs for check, prune, repair, key, and restore refactor(commands): decouple logic from option structs for check, prune, repair, key, and restore Oct 7, 2024
@simonsan simonsan requested a review from aawsome October 7, 2024 00:44
@simonsan simonsan added the S-waiting-for-review Status: PRs waiting for review label Oct 7, 2024
@simonsan simonsan added this to the NEXT milestone Oct 7, 2024
simonsan added a commit that referenced this pull request Oct 7, 2024
Based on #317

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@aawsome aawsome enabled auto-merge October 9, 2024 06:50
@aawsome aawsome added this pull request to the merge queue Oct 9, 2024
Merged via the queue into main with commit 29b2a78 Oct 9, 2024
20 of 22 checks passed
@aawsome aawsome deleted the refactor/decouple_types branch October 9, 2024 07:23
github-merge-queue bot pushed a commit that referenced this pull request Oct 9, 2024
## 🤖 New release
* `rustic_core`: 0.5.1 -> 0.5.2 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

## `rustic_core`
<blockquote>

##
[0.5.2](rustic_core-v0.5.1...rustic_core-v0.5.2)
- 2024-10-09

### Fixed

- *(errors)* use correct error variant for data encryption
([#323](#323))
- *(errors)* handle out of bounds access in PathList display
([#313](#313))
- *(errors)* better error message for hot/cold repo in check
([#297](#297))

### Other

- *(commands)* decouple logic from option structs for check, prune,
repair, key, and restore
([#317](#317))
- *(errors)* improve message and add logging when sending tree from
backend panics
([#314](#314))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

Co-authored-by: rustic-release-plz[bot] <182542030+rustic-release-plz[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience A-testing Area: Testing and coverage C-refactor Category: Refactoring of already existing code S-waiting-for-review Status: PRs waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants