Bug #17503

arvados-client deduplication-report paper cuts

Added by Ward Vandewege 7 months ago. Updated 5 months ago.

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

100%

Estimated time:
Story points:
-
Release relationship:
Auto

Associated revisions

Revision 2b385b54
Added by Ward Vandewege 7 months ago

Merge branch '17503-fix-deduplication-report-paper-cuts'

closes #17503

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 76750532 (diff)
Added by Ward Vandewege 7 months ago

Fix deduplication report test.

refs #17503

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 62b8ef85 (diff)
Added by Ward Vandewege 7 months ago

Simplify code a bit.

refs #17503

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 7 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 7 months ago

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

#3 Updated by Tom Clegg 7 months 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.

#4 Updated by Ward Vandewege 7 months 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

#5 Updated by Tom Clegg 7 months 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

#6 Updated by Ward Vandewege 7 months 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).

#7 Updated by Tom Clegg 7 months ago

LGTM, thanks

#8 Updated by Ward Vandewege 7 months ago

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

#9 Updated by Peter Amstutz 5 months ago

  • Release set to 38

Also available in: Atom PDF