Skip to content

Conversation

@julio-lopez
Copy link
Collaborator

@julio-lopez julio-lopez commented Jul 27, 2024

Followups to #3655
Cleanups, no functional changes.

@codecov
Copy link

codecov bot commented Jul 27, 2024

Codecov Report

Attention: Patch coverage is 79.72973% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.20%. Comparing base (cb455c6) to head (3273a58).
Report is 241 commits behind head on master.

Files Patch % Lines
cli/restore_progress.go 73.46% 8 Missing and 5 partials ⚠️
snapshot/restore/local_fs_output.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4003      +/-   ##
==========================================
+ Coverage   75.86%   77.20%   +1.33%     
==========================================
  Files         470      484      +14     
  Lines       37301    28826    -8475     
==========================================
- Hits        28299    22255    -6044     
+ Misses       7071     4664    -2407     
+ Partials     1931     1907      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@julio-lopez julio-lopez force-pushed the cleanup/restore-progress branch 4 times, most recently from 4ef9010 to e36a60b Compare August 6, 2024 01:37
@julio-lopez
Copy link
Collaborator Author

@e-sumin PTAL

@julio-lopez julio-lopez force-pushed the cleanup/restore-progress branch from e36a60b to 1ba618c Compare August 14, 2024 04:38
@julio-lopez julio-lopez force-pushed the cleanup/restore-progress branch from bcbc99e to a29bbc8 Compare August 23, 2024 17:59
@julio-lopez julio-lopez reopened this Aug 27, 2024
Removes a tautology for `err == nil`, which was guaranteed to be
true in the second case statement for the switch.

Replacing the switch statement with and if/else block is clearer.
Pass arguments in a `restore.Stats` struct.
  `SetCounters(s restore.Stats)`

Simplifies call sites and implementation.

In this case it makes sense to pass all the values
using the restore.Stats struct as it simplifies
the calls.

However, this pattern should be avoided in general
as it essentially makes all the arguments "optional".
This makes it easy to miss setting a value and simply
passing 0 (the default value), thus it becomes error
prone.
In this particular case, the struct is being passed
through verbatim, thus eliminiting the risk of
missing a value, at least in the current state of
the code.
@julio-lopez julio-lopez force-pushed the cleanup/restore-progress branch from a29bbc8 to 3273a58 Compare August 27, 2024 16:19
@julio-lopez julio-lopez requested a review from plar August 27, 2024 16:21
@julio-lopez
Copy link
Collaborator Author

@e-sumin and @plar already reviewed these changes

@julio-lopez julio-lopez marked this pull request as ready for review August 27, 2024 16:40
@julio-lopez julio-lopez merged commit 5dbc8a4 into kopia:master Aug 27, 2024
@julio-lopez julio-lopez deleted the cleanup/restore-progress branch August 27, 2024 16:42
Lyndon-Li pushed a commit to Lyndon-Li/kopia that referenced this pull request Oct 10, 2024
Followups to kopia#3655

* wrap fs.Reader
* nit: remove unnecessary intermediate variable
* nit: rename local variable
* cleanup: move restore.Progress interface to cli pkg
* move cliRestoreProgress to a separate file
* refactor(general): replace switch with if/else for clarity
  Removes a tautology for `err == nil`, which was guaranteed
  to be true in the second case statement for the switch.
  Replacing the switch statement with and if/else block is clearer.
* initialize restoreProgress in restore command
* fix: use error.Wrapf with format string and args


Simplify SetCounters signature:

Pass arguments in a `restore.Stats` struct.
  `SetCounters(s restore.Stats)`
Simplifies call sites and implementation.
In this case it makes sense to pass all the values
using the restore.Stats struct as it simplifies
the calls.
However, this pattern should be avoided in general
as it essentially makes all the arguments "optional".
This makes it easy to miss setting a value and simply
passing 0 (the default value), thus it becomes error
prone.
In this particular case, the struct is being passed
through verbatim, thus eliminating the risk of
missing a value, at least in the current state of
the code.
Lyndon-Li pushed a commit to Lyndon-Li/kopia that referenced this pull request Oct 16, 2024
Followups to kopia#3655

* wrap fs.Reader
* nit: remove unnecessary intermediate variable
* nit: rename local variable
* cleanup: move restore.Progress interface to cli pkg
* move cliRestoreProgress to a separate file
* refactor(general): replace switch with if/else for clarity
  Removes a tautology for `err == nil`, which was guaranteed
  to be true in the second case statement for the switch.
  Replacing the switch statement with and if/else block is clearer.
* initialize restoreProgress in restore command
* fix: use error.Wrapf with format string and args


Simplify SetCounters signature:

Pass arguments in a `restore.Stats` struct.
  `SetCounters(s restore.Stats)`
Simplifies call sites and implementation.
In this case it makes sense to pass all the values
using the restore.Stats struct as it simplifies
the calls.
However, this pattern should be avoided in general
as it essentially makes all the arguments "optional".
This makes it easy to miss setting a value and simply
passing 0 (the default value), thus it becomes error
prone.
In this particular case, the struct is being passed
through verbatim, thus eliminating the risk of
missing a value, at least in the current state of
the code.
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.

1 participant