File Location Flag #2709
File Location Flag #2709
Conversation
cmd/ledger/main.go
Outdated
| if err != nil { | ||
| fmt.Println(err) | ||
| return | ||
| } | ||
| fmt.Printf("\nSuccessfully compared snapshots. Result saved to %s. Total differences found: %v\n", resultFilename, count) | ||
| fmt.Printf("\nSuccessfully compared snapshots. Result saved to %s. Total differences found: %v", resultFilepath, count) |
There was a problem hiding this comment.
Not sure why you removed \n here. The output won't not look nice with the new line. Also, %d fits better instead of %v for count.
Ideally, we should have a test to verify the output that is shown to the user so, these kind of changes don't go unnoticed. You can do that in a separate PR, either by golang Example framework or via a UT by passing the printer
There was a problem hiding this comment.
I must have missed this when I merged since this branch was written before the corrections you made to the original ledger tool utility PR. Removing the newline was not my intention thanks for catching it.
There was a problem hiding this comment.
As far as adding tests for the output, that's something I had in the original iteration but ran into some issues and decided to add that in a future PR, so that's coming up as well.
cmd/ledger/main.go
Outdated
| @@ -24,6 +25,7 @@ var ( | |||
| compare = app.Command("compare", "Compare two ledgers via their snapshots.") | |||
| snapshotPath1 = compare.Arg("snapshotPath1", "File path to first ledger snapshot.").Required().String() | |||
| snapshotPath2 = compare.Arg("snapshotPath2", "File path to second ledger snapshot.").Required().String() | |||
| destination = compare.Flag("destination", "File path to snapshot comparison json results. Default is the current directory.").Short('d').String() | |||
There was a problem hiding this comment.
It's not the File path that you expect here. You use this input as a folder and create a file in that. From that point of view, it would be better to call this flag outputdir
There was a problem hiding this comment.
Similarly, for the existing args, instead of File path to first ledger snapshot first ledger snapshot dir makes it clearer.
FAB-18425 Signed-off-by: Julian Castrence <juliancastrence@ibm.com>
jcastrence
force-pushed
the
compare_snapshots_fileloc
branch
from
dbd9001
to
7e50d23
Compare
Type of change
Description
Related issues