On 27 May 2014 04:02, Tim Penhey <[email protected]> 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 [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
