Merge lp:~axwalk/juju-core/fix-lbox-check into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Remove Logf from .lbox.check

There are two Logfs: one from loggo, and one from gocheck.
They have different positions for their format parameter.

https://codereview.appspot.com/13532047/

Remove Logf from .lbox.check

There are two Logfs: one from loggo, and one from gocheck.
They have different positions for their format parameter.

https://codereview.appspot.com/13532047/

[Andrew Wilkins (axwalk)]

Reviewers: mp+185702_code.launchpad.net,

Message:
Please take a look.

Description:
Remove Logf from .lbox.check

There are two Logfs: one from loggo, and one from gocheck.
They have different positions for their format parameter.

https://code.launchpad.net/~axwalk/juju-core/fix-lbox-check/+merge/185702

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13532047/

Affected files (+3, -1 lines):
   M .lbox.check
   A [revision details]

Index: .lbox.check
=== modified file '.lbox.check'
--- .lbox.check 2013-09-13 08:55:07 +0000
+++ .lbox.check 2013-09-16 05:28:47 +0000
@@ -14,7 +14,7 @@
   -methods \
   -printf \
   -rangeloops \
-
-printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Logf:1,Tracef:0'
\
+
-printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Tracef:0'
\
   .

  # check this branch builds cleanly

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130913152841-ei0xkia5aaifugq1
+New revision: <email address hidden>

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/13532047/

[Roger Peppe (rogpeppe)]

Hmm, good catch. Pity that go vet doesn't take types
into account.

I guess it's not so important because the loggo Logf is
only called in one file, and the other Logf is only used in
tests, but it would be nice if we could keep the coverage.

A different name for loggo.Logger.Logf would seem a reasonable
approach - any suggestions?

On 16 September 2013 06:36, Andrew Wilkins <email address hidden> wrote:
> Reviewers: mp+185702_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Remove Logf from .lbox.check
>
> There are two Logfs: one from loggo, and one from gocheck.
> They have different positions for their format parameter.
>
> https://code.launchpad.net/~axwalk/juju-core/fix-lbox-check/+merge/185702
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/13532047/
>
> Affected files (+3, -1 lines):
> M .lbox.check
> A [revision details]
>
>
> Index: .lbox.check
> === modified file '.lbox.check'
> --- .lbox.check 2013-09-13 08:55:07 +0000
> +++ .lbox.check 2013-09-16 05:28:47 +0000
> @@ -14,7 +14,7 @@
> -methods \
> -printf \
> -rangeloops \
> -
> -printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Logf:1,Tracef:0'
> \
> +
> -printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Tracef:0'
> \
> .
>
> # check this branch builds cleanly
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details] 2012-01-01 00:00:00 +0000
> +++ [revision details] 2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: tarmac-20130913152841-ei0xkia5aaifugq1
> +New revision: <email address hidden>
>
>
>
>
> --
> https://code.launchpad.net/~axwalk/juju-core/fix-lbox-check/+merge/185702
> Your team juju hackers is requested to review the proposed merge of lp:~axwalk/juju-core/fix-lbox-check into lp:juju-core.

[Andrew Wilkins (axwalk)]

>> Hmm, good catch. Pity that go vet doesn't take types
>> into account.
>>
>> I guess it's not so important because the loggo Logf is
>> only called in one file, and the other Logf is only used in
>> tests, but it would be nice if we could keep the coverage.

Yeah, it's not the ideal change. loggo.Logger.Logf is also unlikely to be used directly, given the wrappers.

>> A different name for loggo.Logger.Logf would seem a reasonable
>> approach - any suggestions?

Logf is the natural name, I think. It wouldn't be great to change it to please the tool

Another alternative would be to only vet non-test sources.

=== modified file '.lbox.check'
--- .lbox.check	2013-09-13 08:55:07 +0000
+++ .lbox.check	2013-09-16 05:32:48 +0000
@@ -14,7 +14,7 @@
 	-methods \
 	-printf \
 	-rangeloops \
-	-printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Logf:1,Tracef:0' \
+	-printfuncs 'ErrorContextf:1,notFoundf:0,badReqErrorf:0,Commitf:0,Snapshotf:0,Debugf:0,Infof:0,Warningf:0,Errorf:0,Criticalf:0,Tracef:0' \
 	.
 
 # check this branch builds cleanly