On 28 May 2014 00:46, William Reade <william.re...@canonical.com> wrote:
> Roger
>
> We have had many long discussions, and you have provided much valuable
> input: in particular, explaining to us that we were completely off-base re:
> the attempts to use a slice vs a linked list in the error stacks. And we did
> indeed agree to use errgo (as the only proposed implementation that Did It
> Right in that regard). And you said you'd integrate it, and we agreed an
> approach that I could live with, despite my misgivings re the interface and
> the default behaviour; but, for a variety of perfectly good and excusable
> reasons, this has still not happened after many months.
>
> However, the conditions under which I agreed to use errgo were rather more
> stringent than I feel you have represented. In particular, you did not
> convince me that discard-by-default is a sane approach; you agreed to
> preserve error types by default, in the hope of empirically convincing me
> that, as the codebase evolved, we would end up annotating overwhelmingly at
> boundaries where discarding made sense.
The approach that I was to take was to preserve error types *explicitly*
by default. i.e. each call to Mask would be:
return errgo.Mask(err, errgo.Any)
That means that we would not affect existing code semantics, reducing
the risk of breaking things,
For *new* code, I (still) think that it's best to try to explicitly mention the
errors that are expected to pass through, and mask out the others.
If you're writing new code, you *should* know what kinds of errors
that code is contracted to return (obviously there are cases where
that's bound not to be the case, such as when you're writing code
that explicitly wraps another arbitrary function)
> You observe that this approach fits
> well with current practice, and that's fine, but it fails to take into
> account the fact that our error handling is currently *really bad*.
To attempt to be more precise here, I'd say that our error *handling*
is good - we diagnose errors and act on them as we need to.
Our error *generation* is indeed bad though - our error messages are
uninformative and the errors don't record their return path.
> And this is, IMNSHO, *because* our original annotation practices discard
> types (unless you write tedious verbose site-specific custom code); and so
> we inevitably annotate only where we *have* to, lest we throw away useful
> information; and so it's really hard to actually figure out what really
> happened in a given error situation -- we end up having to grep the source,
> and even when that works unambiguously it still *sucks*. Observing that
> errgo is a flawless drop-in replacement for our historical practices is a
> point *against* its adoption, not *for* it.
I would say that our historical practices are not universally bad.
In the places that we *do* currently annotate, we have a nice guarantee - no
caller can be acting on the basis of the underlying error. That
information is hidden, and as I hope we all agree, decent information hiding
is the basis of well-modularised systems. [1]
I still hope we can end up with the best of both worlds here - a situation where
we can annotate freely *and* hide unnecessary information,
increasing modularity and reducing "action at a distance".
> Still: we *are* leveraging errgo, because it does a lot of things right. As
> Tim said, the fact that we can implement what we want in errgo is a strong
> endorsement of its quality and flexibility. But the philosophy underlying
> errgo's interface does not play nicely with the direction I want to take the
> project [0], while the arrar-style interface does, and so we wrap errgo such
> that it fits our needs (which Tim has already clearly described).
>
> I don't want to appear to annex ownership of errgo; it's your package, and
> it has a cool name, and I wish it popularity and widespread adoption. If you
> feel that juju/errgo makes for a counterproductive canonical location, we
> can happily thirdparty it in its current state; and you can write your own
> error handling code outside juju, using an errgo outside juju, as you wish.
> We could even rename our version if you want [1]. But I will not stand for
> further delay and confusion and the associated fossilization of poor
> practices in the codebase for which I bear responsibility.
>
> Re unsafe: where we can't check equality, we fall back on identity. We know
> it's a hack; and if it's fundamentally broken, we'd like to know why, so we
> can fix it. Please clarify your objections.
Comparing the internal interface structs compares neither equality nor
identity. http://play.golang.org/p/1_fdhPdWBf
A slightly better approach would be something like this, but it's
possible that it might not play well with gccgo:
http://play.golang.org/p/3SMiSjW023
Fundamentally though, I not sure that cutting off half the error message because
the cause has changed is a great approach.
> Cheers
> William
>
> [0] ...which I am not interested in rehashing. My considered opinion is
> that, while both approaches have potential pathological failure situations,
> my experience across projects indicates that the ones that come up in
> practice overwhelmingly favour discarding error types only when we're sure
> it's a good idea to do so; writing code to explicitly preserve specific
> errors at specific times will be a perpetual drag on clean refactoring, and
> will rapidly lead to an accumulation of meaningless cruft that nobody has
> the courage to remove just in case some distant module still depends upon
> it.
Writing code to explicitly declare function return types is also
a perpetual drag on refactoring, but it's also incredibly useful.
The set of possible returned errors *should* be part of the contract
of a function, otherwise how could you know that a given error might
be returned?
The cruft isn't meaningless - it actually expresses the intent of the contract.
The alternative is the situation we have now, where I cannot change
the implementation of any function for fear that some distant module
depends on a specific error type returned by one of the functions
I'm no longer calling. This is a current reality in our code base, and
I know of areas that are highly vulnerable to this kind of breakage.
If the "cruft" is there, there should be a test for it. If there's a test for
it, it can't be meaningless because it can actually happen.
It's no more meaningless than the documentation for json.Marshal
documenting that it returns an UnsupportedTypeError when marshalling
a channel. Perhaps no-one ever checks for that error (in the case
of json.Marshal, I suspect that's true), but it's part of the contract,
and as such, it's useful annotation, not cruft.
Anyway, you are not the only one that's tired of this conversation.
It is good that juju-core is using errgo underneath, because
that at least means that when I write code in another project,
it will interoperate, even if that other project does not use
github.com/juju/errors.
I hope it will eventually become clear that the errgo approach is
both feasible and useful.
cheers,
rog.
[1] It's particularly important when module interconnection cannot
be determined statically, as is the case for errors.
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
