Make LazyMultiError only constructable via function

Make LazyMultiError only constructable via function

Followup to https://codereview.chromium.org/1249933002/

R=tandrii@chromium.org, dnj@chromium.org, estaab@chromium.org, maruel@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/1318cdbe2db8d755e15a9de8075c412325d30351

[M-A Ruel, 2015-08-12 19:13:50]

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go
File common/errors/multierror.go (right):

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go...
common/errors/multierror.go:84: type LazyMultiError struct {
Why doesn't LazyMultiError have method Error() so it would fit the error
interface?

[iannucci, 2015-08-12 22:06:07]

On 2015/08/12 19:13:50, M-A Ruel wrote:
> https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go
> File common/errors/multierror.go (right):
> 
>
https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go...
> common/errors/multierror.go:84: type LazyMultiError struct {
> Why doesn't LazyMultiError have method Error() so it would fit the error
> interface?

Because Error() doesn't allow you to be a nil error (which callers check for).
It's never helpful to pass a LazyMultiError as an error, because it will always
fulfil the Error() interface, even if it contains no actual error. The Get()
method returns an error interface (which is either `nil` or a MultiError). e.g.
http://play.golang.org/p/SelUuPS9he

[iannucci, 2015-08-14 02:11:36]

can to stamp pls? n e 1?

[tandrii_google, 2015-08-14 08:50:36]

tandrii@google.com changed reviewers:
+ tandrii@google.com

[tandrii_google, 2015-08-14 08:50:36]

https://chromiumcodereview.appspot.com/1284323002/diff/1/common/errors/multie...
File common/errors/multierror.go (right):

https://chromiumcodereview.appspot.com/1284323002/diff/1/common/errors/multie...
common/errors/multierror.go:87: size int
wait, this doesn't make what CL description says. ie I can still do:
v := NewLazyMultiError{}

and I tested just that here (pcg is happy):
https://codereview.chromium.org/1297573002 .

So, either fix description, or make LazyMultiError an interface.

[iannucci, 2015-08-14 15:41:34]

Yeah you're right, it should be an interface now. I'll fix.

On Fri, Aug 14, 2015, 01:50  <tandrii@google.com> wrote:

>
>
>
https://chromiumcodereview.appspot.com/1284323002/diff/1/common/errors/multie...
> File common/errors/multierror.go (right):
>
>
>
https://chromiumcodereview.appspot.com/1284323002/diff/1/common/errors/multie...
> common/errors/multierror.go:87
>
<https://chromiumcodereview.appspot.com/1284323002/diff/1/common/errors/multie...>:
> size int
> wait, this doesn't make what CL description says. ie I can still do:
> v := NewLazyMultiError{}
>
> and I tested just that here (pcg is happy):
> https://codereview.chromium.org/1297573002 .
>
> So, either fix description, or make LazyMultiError an interface.
>
> https://chromiumcodereview.appspot.com/1284323002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dnj (Google), 2015-08-14 16:40:00]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-08-14 16:40:01]

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go
File common/errors/multierror.go (right):

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go...
common/errors/multierror.go:84: type LazyMultiError struct {
If your goal is "only constructable", then LazyMultiError should be an interface
w/ an implementing private struct. Currently, I can instantiate a (admittedly
not so useful) one via LazyMultiError{}.

[iannucci, 2015-08-14 17:00:26]

PTAL, moved LME to new file as well

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go
File common/errors/multierror.go (right):

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go...
common/errors/multierror.go:84: type LazyMultiError struct {
On 2015/08/14 16:40:00, dnj (Google) wrote:
> If your goal is "only constructable", then LazyMultiError should be an
interface
> w/ an implementing private struct. Currently, I can instantiate a (admittedly
> not so useful) one via LazyMultiError{}.

Done

https://codereview.chromium.org/1284323002/diff/1/common/errors/multierror.go...
common/errors/multierror.go:87: size int
On 2015/08/14 08:50:36, tandrii_google wrote:
> wait, this doesn't make what CL description says. ie I can still do:
> v := NewLazyMultiError{}
> 
> and I tested just that here (pcg is happy):
> https://codereview.chromium.org/1297573002 .
> 
> So, either fix description, or make LazyMultiError an interface.

done

[tandrii(chromium), 2015-08-14 17:14:30]

lgtm % nit

https://codereview.chromium.org/1284323002/diff/20001/common/errors/lazymulti...
File common/errors/lazymultierror.go (right):

https://codereview.chromium.org/1284323002/diff/20001/common/errors/lazymulti...
common/errors/lazymultierror.go:18: // Build one with NewLazy.
nit: NewLazyMultiError

[iannucci, 2015-08-14 18:39:17]

Committed patchset #3 (id:40001) manually as
1318cdbe2db8d755e15a9de8075c412325d30351 (presubmit successful).