Skip to content

Conversation

@hollowhemlock
Copy link
Contributor

Addresses #526 . Clarifies meaning of 0.

@coderabbitai
Copy link

coderabbitai bot commented Sep 6, 2025

Walkthrough

Updated help text in restic/commands.json for six forget options to clarify default value behaviour and add a docs link; adjusted wording in docs/content/configuration/sleep.md to specify that prevent-sleep applies while running commands. No behavioural or API changes.

Changes

Cohort / File(s) Summary of Changes
Option description updates
restic/commands.json
Amended description text for forget options keep-daily, keep-hourly, keep-last, keep-monthly, keep-weekly, keep-yearly to note that the default value of 0 removes no snapshots and to add a documentation link.
Documentation wording tweak
docs/content/configuration/sleep.md
Clarified that prevent-sleep applies "while running commands" rather than generally; no other content changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Areas to spot-check:

  • The updated sentences for accuracy and grammar in restic/commands.json.
  • The prevent-sleep wording in docs/content/configuration/sleep.md for contextual consistency with surrounding documentation.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: clarifying that forget command options with value 0 don't remove snapshots, with a reference to issue #526.
Description check ✅ Passed The description is related to the changeset, referencing issue #526 and indicating the change clarifies the meaning of value 0 for forget options.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1159ec1 and 7d72c12.

📒 Files selected for processing (1)
  • docs/content/configuration/sleep.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • docs/content/configuration/sleep.md

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (7)
restic/commands.json (7)

822-822: Prefer clearer phrasing: “0 disables this policy; it does not remove snapshots.”

The current sentence could be misread as an action (“removes no snapshots”). Suggest rewording for precision and consistency with restic’s behaviour.

-        "Description": "keep the last n daily snapshots (use 'unlimited' to keep all daily snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n daily snapshots (use 'unlimited' to keep all daily snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

831-831: Same clarity tweak for hourly retention.

-        "Description": "keep the last n hourly snapshots (use 'unlimited' to keep all hourly snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n hourly snapshots (use 'unlimited' to keep all hourly snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

840-840: Same clarity tweak for “keep-last”.

-        "Description": "keep the last n snapshots (use 'unlimited' to keep all snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n snapshots (use 'unlimited' to keep all snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

849-849: Same clarity tweak for monthly retention.

-        "Description": "keep the last n monthly snapshots (use 'unlimited' to keep all monthly snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n monthly snapshots (use 'unlimited' to keep all monthly snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

867-867: Same clarity tweak for weekly retention.

-        "Description": "keep the last n weekly snapshots (use 'unlimited' to keep all weekly snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n weekly snapshots (use 'unlimited' to keep all weekly snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

930-930: Same clarity tweak for yearly retention.

-        "Description": "keep the last n yearly snapshots (use 'unlimited' to keep all yearly snapshots). The default value of 0 removes no snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",
+        "Description": "keep the last n yearly snapshots (use 'unlimited' to keep all yearly snapshots). A value of 0 disables this policy and does not remove any snapshots. See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots",

822-822: Consider pinning to the “stable” docs rather than “latest”.

Linking to “stable” avoids churn if “latest” changes semantics or anchors. Optional, as other entries already use various external links.

- See https://restic.readthedocs.io/en/latest/060_forget.html#removing-all-snapshots
+ See https://restic.readthedocs.io/en/stable/060_forget.html#removing-all-snapshots

Also applies to: 831-831, 840-840, 849-849, 867-867, 930-930

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a761a7b and 1159ec1.

📒 Files selected for processing (1)
  • restic/commands.json (6 hunks)
🔇 Additional comments (1)
restic/commands.json (1)

768-1025: LGTM — helpful clarification without behavioural change.

The additions make the defaults explicit and reference the right section in the docs.

@creativeprojects
Copy link
Owner

Hey! Thank you for the clarification.

This file commands.json is actually generated from the restic documentation itself (after running restic generate --man).

It's going to be a bit move involved to add this message: in the documentation parser, we need to add it to the existing description from the manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants