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. 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*. 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. 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. 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. [1] Maybe we could call it arrar... On Tue, May 27, 2014 at 12:04 PM, roger peppe <roger.pe...@canonical.com>wrote: > On 27 May 2014 04:02, Tim Penhey <tim.pen...@canonical.com> wrote: > > On 23/05/14 02:47, roger peppe wrote: > >> Any particular reason to wrap the errgo functions rather than using > errgo > >> directly? Personally, I see the role of the errors package to be about > >> specific error classifications (something that errgo tries hard to be > entirely > >> agnostic about), but errgo could, and I believe should, be used > directly for all > >> the more general calls. > > > > Sure. There were several reasons that kept me away from using errgo > > directly or modifying errgo. > > > > 1) Probably the most important, I don't feel that errgo is a 'juju' > > library but very much a personal library of yours, one that you have > > very strong feelings about, and I feel that proposing changes that > > fundamentally change how it behaves will get bogged down in > > bike-shedding and disagreement. I was wanting to get the behaviour that > > we wanted, and I found a way to get that behaviour without touching > > errgo, so I went that way. > > errgo is now under github.com/juju precisely because I wanted to bring it > into > the juju fold - it's there *for* juju and it should do what juju needs. > > It's true that I have definite preferences for how some of the behaviour > works, because I have thought a lot about the emergent effects of > the way we handle and generate errors in juju, and I believe that > a using errgo's approach consistently will lead to code that > is easier to maintain and understand. See > > http://rogpeppe.wordpress.com/2014/04/23/some-suggested-rules-for-generating-good-errors-in-go/ > for some background on that. > > I understand that you don't agree entirely with the approach, > but there *was* a long discussion in which several of us ended up agreeing > a) to use errgo > b) on the naming scheme that it currently uses. > > Rather than unilaterally deciding to do things differently because > you don't agree with those decisions, wouldn't it be better > to seek some agreement first, as I did? > > > 2) The default behaviour of errgo to mask the underlying cause was not > > the behaviour that juju wants. For example the Notef function returns > > an error where the Cause of that error is different to the Cause of the > > error that is being annotated. I know that there is NoteMask, but that > > isn't what I think of when all I want to do is add a note. > > When you're writing the code, you know what kinds of > errors you want to pass back ("we just want to annotate, and leave the > NotFound error as is"), but when reading the code later, that > is obscure. I still believe that it's better to *explicitly* annotate the > code with that knowledge at each level. > > In the vast majority of cases (I know this empirically) we do *not* > want or need to pass back the error cause. Look at all the current > uses of fmt.Errorf(...., err) - none of them passes back the error cause, > and that's a Good Thing. We have had subtle bugs because > error causes for two very different things were conflated. > > I think it's a reasonable principle that the behaviour we want most often > should > be the default. > > > 3) I found the functions in errgo to be confusing when what I wanted was > > three simple things: add stack information, annotate the error (and add > > stack info), or change the cause (and add stack info). There is no way > > to just change the cause with errgo without providing some annotation, > > and I think that if you are a user of the library and wanting to provide > > a more detailed error to describe the problem, then that error is likely > > to contain all the information that you want to see, and as such, > > forcing them to also add a message is not needed. > > You don't *need* to add a message - just pass the empty string > to WithCausef and no message will be added. It should > probably be documented that it's OK to do that - or if this > functionality is used a lot, a WithCause function would work > just fine. FWIW, when I converted the whole of juju to use > errgo, I didn't use that function once. If you're changing the > cause, you probably want some annotation to reflect that > though (but see below). > > > 4) The string representation of the error was not what we wanted with > > juju. The errgo implementation walks up the entire error stack > > outputting annotations up to the top, and then the underlying error. > > The behaviour we wanted was to walk up the annotations and stop when the > > cause of the error changes and write out the cause. That way actually > > changing the cause of the error as it is passed down but keeping the > > entire call stack gives a string representation of the actual (most > > recent) cause of the error, not the original, and yet the entire call > > stack could be output. > > I have definitely struggled to find a good behaviour here for errgo. > The problem is that Cause is orthogonal to Underlying, and > you really want to be able to see *both* in the error string output, > because both are important. > > The reason I chose the solution that I did was that the "underlying error" > is really the thing that is designed for the user to see, so we show that > to the user and we can try to ensure that it will appear meaningful when > shown; > the "cause" is the thing that's designed for the program to analyse, so > we only show that when debugging (and it can be shown by the Details > function - there's definitely an argument to add an argument to enable > the cause to be shown there, rather than having it turned on by > the debug constant). > > I would like to see some concrete examples of when one case > or the other would be useful - this is something that can be > easily adjusted in the light of experience. > > By the way, the code that currently implements the "cause has changed" > logic inside juju/error is a) bad because it imports unsafe, which > we should never do unless strictly necessary, and this is the > only place we do so inside the whole of juju except for one unavoidable > windows system call and b) broken. > > > Getting the stack trace information accessible to debugging and logging > > was my first priority. The errgo discussion started almost a year ago > > and we still hadn't gotten things merged and it was my understanding > > that a key reason for that was a difference of opinion around the 'keep > > the cause the same' vs. 'the cause of the error is different' default > > behaviour. I was not interested in getting into yet more discussions > > and time delay around getting the functionality we want into the juju > > libraries. > > I have had it ready to merge several times - the last couple of times > it did not was because of very small issues. As far as I am aware > there was no remaining difference of opinion - we had agreed on > the approach. > > > I was wanting to move the error package out of juju-core any way so it > > could be used by other projects (more easily), and it just seemed timely > > to add the, in my opinion, simpler interface to that package. > > I think that moving the error package out of juju-core is a good idea. > As outlined above, I don't think reimplementing the arrar primitives > on top of errgo (awkwardly) is a good idea. > > cheers, > rog. > > -- > Juju-dev mailing list > Juju-dev@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev >
-- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
