Arvados

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

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.

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

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

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).

LGTM, thanks