Project

General

Profile

Actions

Bug #17503

closed

arvados-client deduplication-report paper cuts

Added by Ward Vandewege almost 2 years ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Story points:
-
Release relationship:
Auto
Actions #1

Updated by Ward Vandewege almost 2 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Ward Vandewege almost 2 years ago

Ready for review at 661483adb0993714e04bff7c1d3efbfa85ca9cca on branch 17503-fix-deduplication-report-paper-cuts

Actions #3

Updated by Tom Clegg almost 2 years ago

The parseFlags() exitcode return value means 0=ok, 1=exit 0 now, 2=exit 2 now, which seems unnecessarily obscure. How about returning an error value instead, and check in report() whether err==flag.ErrHelp?

Rest LGTM, thanks.

Actions #4

Updated by Ward Vandewege almost 2 years ago

Tom Clegg wrote:

The parseFlags() exitcode return value means 0=ok, 1=exit 0 now, 2=exit 2 now, which seems unnecessarily obscure. How about returning an error value instead, and check in report() whether err==flag.ErrHelp?

Yes! Changes made in @commit:da29f21261586780cc13678134eaa39da8fe8e50 on branch 17503-fix-deduplication-report-paper-cuts

Actions #5

Updated by Tom Clegg almost 2 years ago

Hm... I was thinking of only returning an error, not exitcode -- essentially the same approach as flag.Parse(). Returning both seems kind of awkward.

This should either use logger.Error(err.Error()) or logger.Errorf("%s", err) ... it will do odd things if the error message has a % char:

                       logger.Errorf(err.Error())

Not sure why you added newlines to error messages -- logger is already line-oriented and shouldn't need them?

nit: I think parseFlags would be more readable if it didn't use named return args -- just say "return nil, err" in the error cases, and "return inputs, nil" on success. DC is not a big fan, fwiw... https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_named_return_values

Actions #6

Updated by Ward Vandewege almost 2 years ago

Tom Clegg wrote:

Hm... I was thinking of only returning an error, not exitcode -- essentially the same approach as flag.Parse(). Returning both seems kind of awkward.

True, fixed.

This should either use logger.Error(err.Error()) or logger.Errorf("%s", err) ... it will do odd things if the error message has a % char:

Indeed, also fixed thanks.

Not sure why you added newlines to error messages -- logger is already line-oriented and shouldn't need them?

You're right, the newline was missing from the

func (f *NoPrefixFormatter) Format(entry *logrus.Entry) ([]byte, error)

function, this is better, thanks.

nit: I think parseFlags would be more readable if it didn't use named return args -- just say "return nil, err" in the error cases, and "return inputs, nil" on success. DC is not a big fan, fwiw... https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_named_return_values

Ah, good point, also fixed.

Latest in 5fc96ed11f3b6312f09ecb34417eb9eac6e69a32 on 17503-fix-deduplication-report-paper-cuts (a squashed rebase, sorry).

Actions #7

Updated by Tom Clegg almost 2 years ago

LGTM, thanks

Actions #8

Updated by Ward Vandewege almost 2 years ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions #9

Updated by Peter Amstutz over 1 year ago

  • Release set to 38
Actions

Also available in: Atom PDF