json output forget command: added id's in snapshots within reasons object#4737
json output forget command: added id's in snapshots within reasons object#4737MichaelEischer merged 1 commit intorestic:masterfrom stephan0307:3117
Conversation
MichaelEischer
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a few small remarks, see below.
cmd/restic/cmd_forget.go
Outdated
| Matches []string `json:"matches"` | ||
| } | ||
|
|
||
| func addJSONKeeps(js *[]KeepReason, list []restic.KeepReason) { |
There was a problem hiding this comment.
Please change the function signature to addJSONKeeps(list []restic.KeepReason) []KeepReason. There's absolutely no need to use an output parameter here.
There was a problem hiding this comment.
With a focus on clean code and easy readability, I think it is more important to use the same solutions for the same requirements.
That's why I did it according to the following existing function only a few lines above:
func addJSONSnapshots(js *[]Snapshot, list restic.Snapshots)
There was a problem hiding this comment.
I changed for both functions the signature as suggested:
- asJSONSnapshots(list restic.Snapshots) []Snapshot
- asJSONKeeps(list []restic.KeepReason) []KeepReason
It is the first time for me doing a PR, so i have a Questions:
Should i squash the fixes with the first commit?
There was a problem hiding this comment.
I didn't notice that addJSONSnapshots was doing strange things :-) . Thanks for cleaning it up!
Yes please squash the commits (whether or not commits are squashed and when usually differs a lot between open source projects). Btw, I've that you've used to different names/emails in the commits. You might want to check which one should get merged.
There was a problem hiding this comment.
squash done and used correct e-mail (thanks for your notic)
There was a problem hiding this comment.
fork syncronized and commit rebased on head
| @@ -0,0 +1,3 @@ | |||
| Enhancement: include snapshot id in reason field of forget JSON output | |||
There was a problem hiding this comment.
The filename should be pull-4737
changelog/unreleased/issue-4737
Outdated
| @@ -0,0 +1,3 @@ | |||
| Enhancement: include snapshot id in reason field of forget JSON output | |||
|
|
|||
| The JSON output of the `forget` command now includes the `id` and `short_id` of a snapshot in the `reason` field. No newline at end of file | |||
There was a problem hiding this comment.
Please add a link to this PR as the last line of the changelog:
https://github.com/restic/restic/pull/4737
In order to evaluate the keep reasons for snapshots, there should be also the id's to compare it with snapshots within the keep object. (See also Issue #3117) In order to avoid output parameters also changed function addJSONSnapshots to asJSONSnapshots
What does this PR change? What problem does it solve?
In order to evaluate the reasons for keep snapshots, there should be also
the id's within the snapshots in the reasons object to have exact reference.
(See also Issue #3117)
Was the change previously discussed in an issue or on the forum?
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.