Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 29 May 2014 21:48, William Reade william.re...@canonical.com wrote:
Masking is often necessary, and information hiding is valuable, and
depending on spooky action at a distance is generally foolish. But the
granularity with which we want to track the propagation of a given error is
*much* finer than the granularity of judicious information hiding: the error
is likely to pass through a lot more stack frames than it is package
boundaries, and the former transitions are the ones we need to track to
properly diagnose errors [0], while the latter are the ones we need to mask
to properly maintain global sanity [1].
Isn't it better to maintain sanity in this respect at
a function/method level? It is very common to move functions between
packages, and having structures that maintain correctness when
doing this is very useful.
I would argue that by maintaining and documenting error types
at a function level, we make intra-package refactoring easier too.
It's a simple rule: document and notate the kinds of errors that may
be returned by a given function. If it's not documented, don't rely on it.
Package boundaries are fluid over time - functions get exported and unexported
and moved between packages. If we have a firm convention that *all*
functions document and notate their possible error types (some may
of course document that the possible error types are open-ended
and depend on some component that's a parameter), then
we'll have a better and more maintainable system. See below
for an example.
To mask by default, at every annotation point, leads quickly to code whose
action is characterised by bizarre and mystifying errors [2].
I don't get this at all. Bizarre and mystifying errors are, in my experience,
caused by errors that are not sufficiently annotated, not errors without
a diagnosable cause. Masking by default has no impact on the former;
it changes only the latter.
To *fail* to
mask at sensible boundaries also leads to bugs, and we've seen one or two of
those as well; the trouble is that masking is *always easy*, while pure
annotation is (relatively) hard.
On the contrary, if everyone now uses errors.Annotate, masking is harder
because that preserves the cause by default. To mask the cause, you'd have to
explicitly do:
return errors.Wrap(errors.Annotate(err, annotation), nil)
This is not only considerably more verbose - it adds another
unnecessary level to the error stack.
---
I'll try to illustrate the points I am trying to make more concretely
by referring to the juju-core source.
For example, see this line of code at
http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/environs/imagemetadata/generate.go#L43
:
existingMetadata, _, err := Fetch(
[]simplestreams.DataSource{dataSource},
simplestreams.DefaultIndexPath, imageConstraint, false)
if err != nil !errors.IsNotFound(err) {
return nil, err
}
If the condition is triggered, the code falls back to existing metadata.
The question is: under what circumstances will the condition
(errors.IsNotFound(err))
be triggered - is this code actually correct?
The only way to find out is to exhaustively inspect the code paths reachable
from the Fetch call, truncating the search at any return that obviously
cannot return a NotFound error.
I did that. Here's a summary of what I found: http://paste.ubuntu.com/7551096/
As far as I can see, the circumstances in which this condition can be triggered
are:
a) if a DataSource.Fetch call fails for any reason.
b) if simplestreams.IndexReference.GetProductsPath returns NotFound.
These both seem somewhat reasonable (although I'm reasonably sure
that a network failure should not trigger the not found behaviour).
But from readMetadata to GetProductsPath are 5 levels of call stack,
4 of which are exported functions, *none of which document or test
any of their error return possibilities*.
environs/imagemetadata.readMetadata
environs/imagemetadata.Fetch
environs/simplestreams.GetMetadata
environs/simplestreams.getMaybeSignedMetadata
environs/simplestreams.IndexReference.GetCloudMetadataWithFormat
environs/simplestreams.IndexReference.GetProductsPath
If any of those intermediate functions are unwittingly changed to mask
the NotFoundError or to call a function that may return a NotFoundError,
our code will be broken.
Note that the only reason it was actually feasible to do this analysis
*at all* was because there are many places where the underlying error was
masked, enabling me to truncate the search. With pervasive non-masking
of errors, as suggested, this will no longer be true. The juju/errors changes
will actually make things worse (from a logic-verifiability point of view)
than the current situation.
If someone changes simplestreams.SeriesVersion to return a NotFoundError
when the series is not found (a superficially reasonable change) it will
break
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 29 May 2014 20:00, Jeroen Vermeulen jeroen.vermeu...@canonical.com wrote: On 2014-05-29 09:50, roger peppe wrote: On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. It may help to consider this as two problems. One: caller needs to know about a specific failure — e.g. because it's a failure to the callee but not to the caller. Definitely part of the contract. You either: (a) define a super-specific error (exception class, error code, etc.), or (b) document that standard error X means failure Y in this case, and the caller picks up the error as close as possible to its origin. With these errors you have to make sure that the information isn't diluted as it propagates, but usually you don't have to take it too far up the call chain. This is the kind of error I have been thinking about. Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. This, on the other hand is interesting, and something I have not really considered. Thanks for bringing it up. I haven't written code like this, and I'm not aware of any in the juju-core code base. Have you got any real-world examples you could point to? FWIW, this approach is still possible even when error types are masked by default - all the error stack information is still exposed, even though convention would suggest that it should not be looked at, just as it's a useful convention that we should not base program behaviour on the string contents of error messages. cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 2014-05-30 15:08, roger peppe wrote: Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. This, on the other hand is interesting, and something I have not really considered. Thanks for bringing it up. I haven't written code like this, and I'm not aware of any in the juju-core code base. Have you got any real-world examples you could point to? Usually it's mundane stuff. I'm sure you're familiar with this and I just described in a way that made it harder to recognise! Think of things like: * A broken-connection error can emerge from any of a number of places below this function call, but you've got a good way of handling them right after you come out of the call. If you can't recognise the error for whatever reason, oh well, the user experience degrades a bit but you handle it as a generic program failure. * An IMAP client library annotates some kinds of errors with details about the command that failed. It can't decide for you that you'll want those details in the error message, but you may still want to log them elsewhere. You don't need that on a DNS lookup error, for example, so for that type of error you don't much care if you log the details or not. It won't bother you if the library returns a different kind of error than before in some situations. * In some error situations a failed database transaction can be retried, and may well succeed next time. This can save you a transient failure. Of course it can be difficult to tell whether it's safe to do so. Some types of database errors can say retrying is safe as far as I'm concerned, but you default to failure. * You've been writing a file that could get quite large. If you fail before you're done, you want to keep the data you have. But if the error was disk full, you prefer to clean up the output as a kindness to the user. * My systems management library calls a library called CoolApt to run the equivalent of apt-get update. Your application uses my library. It knows about the CoolApt can't acquire packages lock error and does a bit of investigating to provide a better explanation to the user. When version 2.0 of my library replaces CoolApt with HotApt, you obviously want to upgrade. The error is different, so this feature is temporarily broken — but you may find that acceptable. These are all quality of service distinctions. The effort of a more specific error is appreciated, but you can accept a certain amount of change. Jeroen -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: On 29/05/14 01:11, roger peppe wrote: 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? This theory has also been applied to both Java and C++ in the past with checked error types, and in both situations it ended up being unworkable and discarded by both communities. There's a huge difference between adding errors formally as part of the type system and checking them as part of the runtime. The latter is indeed unworkable because you end up with a proliferation of method signatures that cannot work together. The former is more like a domain-specific form of Eiffel's postconditions. To put it another way, we don't try to encode the fact that math.Sqrt returns the square root of a number in its type signature. But that fact is definitely part of the contract of the function, and we write tests for it. Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. That already happens, and it has resulted in real bugs. The errgo stuff is trying to be a way of communicating that thinking in code and it defaults to a mode that closes off those arbitrary communication channels by default, reducing the overall complexity of the system. I am by no means saying that the errgo interface is perfect, or even particularly great - the API design feels cluttered and unnecessarily complex even though the code is quite simple. But I stand by that aspect of the design. cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 28 May 2014 19:40, Nate Finch nate.fi...@canonical.com wrote: I think the main problem with errgo as it exists, is that it is very awkward to allow error type information to pass through: return errgo.Mask(err, errgo.Any) This line is very misleading. It's saying - mask the error type, except wait, actually don't mask the type. It's never a good idea to use a function to do the opposite of what it's name says it does. That's partly why I originally called the function Wrap, but was persuaded to change it to Mask by popular request. And, I've said this before and I'm sure I'll say it again - it's actually very rare to need to return *any* error type information to pass through. When I changed all of juju to work this way, there were about 5 occurrences out of hundreds, and all were for good and understandable reasons. Instead, it would be much better to have a method like this: errgo.MaintainErrorTypeAndAddTraceback(err) (with a better name, obviously, but that's not the point) We could just call it Wrap. For the record, I still find the API of errgo to be baffling without a long ramp up period. Having both an underlying error and a cause is hard to wrap my head around. I finally managed to load it all up out of long term storage, so I get it again... but it's not a terribly easy package to understand, in my opinion. I think this is a valid criticism. I agree that having two entirely orthogonal errors is awkward (and Tim's struggles with producing an intuitive error message demonstrate that). I have been toying with the idea of always maintaining a single error stack, but noting the cause by keeping track of its depth in the stack. So to change the cause, you push an error on the stack that has a cause-depth of zero. If you don't need to change the cause, you push an error that has a cause-depth of one more than the cause-depth of the current top-of-stack. To extract or check the cause, you traverse cause-depth levels down the stack and return the error found there. (To make this more efficient, you could store the cause at each level) That means that the cause can be annotated by simply emphasising (with an asterisk or whatever) the error in the stack that's the current cause. The API could remain largely the same, with the exception that WithCausef would go. We'd probably unexport the Underlying_ and Cause_ fields from the Err struct type too. I think I quite like this idea - what do you think? -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 2014-05-29 09:50, roger peppe wrote: On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. It may help to consider this as two problems. One: caller needs to know about a specific failure — e.g. because it's a failure to the callee but not to the caller. Definitely part of the contract. You either: (a) define a super-specific error (exception class, error code, etc.), or (b) document that standard error X means failure Y in this case, and the caller picks up the error as close as possible to its origin. With these errors you have to make sure that the information isn't diluted as it propagates, but usually you don't have to take it too far up the call chain. Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. For example, if you're writing code which speaks to another program over the network, you may want to know: is this connection still usable? Do I know what happened to the operation I requested? Were we able to connect in the first place? Am I doing something that shouldn't ever work, such as mis-spell a command? With these errors you act on the best information you have, but you can always just fail. Jeroen -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
What Roger is talking about, is when an error is implementation-specific and may change if the implementation changes. For example, if you have a black box service that saves data, it might write to the network or it might write to a filesystem, but where it saves is an implementation detail. Let's say in the current implementation, Save() returns a FileNotFoundError if you haven't called Initialize() first, then people may write code that handles a FileNotFoundError by calling Initialize() now if you change the implementation to write to the network, so now if they haven't called Initialize(), you return a NetworkNotFoundError in the same situation their code will break. If you had instead written your own error, or just masked the type of the error to something generic, then they wouldn't have been able to write their code that makes bad assumptions. Note that this is the most simple case of the problem. In reality, it can be that some sub-sub-sub package was returning that specific error, and your code had no idea, it just passed it on up unchanged. This is where it gets insidious, because *your* code didn't change. Some dependency of a dependency changed that you might not even realize you were calling. And now people using your code are broken. If you masked all returned errors except the ones you explicitly wanted to return, then this couldn't happen. On Thu, May 29, 2014 at 3:00 PM, Jeroen Vermeulen jeroen.vermeu...@canonical.com wrote: On 2014-05-29 09:50, roger peppe wrote: On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. It may help to consider this as two problems. One: caller needs to know about a specific failure — e.g. because it's a failure to the callee but not to the caller. Definitely part of the contract. You either: (a) define a super-specific error (exception class, error code, etc.), or (b) document that standard error X means failure Y in this case, and the caller picks up the error as close as possible to its origin. With these errors you have to make sure that the information isn't diluted as it propagates, but usually you don't have to take it too far up the call chain. Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. For example, if you're writing code which speaks to another program over the network, you may want to know: is this connection still usable? Do I know what happened to the operation I requested? Were we able to connect in the first place? Am I doing something that shouldn't ever work, such as mis-spell a command? With these errors you act on the best information you have, but you can always just fail. Jeroen -- 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
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
Note that this kind of error masking doesn't need to be done in the same package as the base error package. I made a little standalone package http://godoc.org/github.com/natefinch/filterr that does it, we could do something similar if we wanted to keep the error package simpler (which is always a good thing). On Thu, May 29, 2014 at 3:16 PM, Nate Finch nate.fi...@canonical.com wrote: What Roger is talking about, is when an error is implementation-specific and may change if the implementation changes. For example, if you have a black box service that saves data, it might write to the network or it might write to a filesystem, but where it saves is an implementation detail. Let's say in the current implementation, Save() returns a FileNotFoundError if you haven't called Initialize() first, then people may write code that handles a FileNotFoundError by calling Initialize() now if you change the implementation to write to the network, so now if they haven't called Initialize(), you return a NetworkNotFoundError in the same situation their code will break. If you had instead written your own error, or just masked the type of the error to something generic, then they wouldn't have been able to write their code that makes bad assumptions. Note that this is the most simple case of the problem. In reality, it can be that some sub-sub-sub package was returning that specific error, and your code had no idea, it just passed it on up unchanged. This is where it gets insidious, because *your* code didn't change. Some dependency of a dependency changed that you might not even realize you were calling. And now people using your code are broken. If you masked all returned errors except the ones you explicitly wanted to return, then this couldn't happen. On Thu, May 29, 2014 at 3:00 PM, Jeroen Vermeulen jeroen.vermeu...@canonical.com wrote: On 2014-05-29 09:50, roger peppe wrote: On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. It may help to consider this as two problems. One: caller needs to know about a specific failure — e.g. because it's a failure to the callee but not to the caller. Definitely part of the contract. You either: (a) define a super-specific error (exception class, error code, etc.), or (b) document that standard error X means failure Y in this case, and the caller picks up the error as close as possible to its origin. With these errors you have to make sure that the information isn't diluted as it propagates, but usually you don't have to take it too far up the call chain. Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. For example, if you're writing code which speaks to another program over the network, you may want to know: is this connection still usable? Do I know what happened to the operation I requested? Were we able to connect in the first place? Am I doing something that shouldn't ever work, such as mis-spell a command? With these errors you act on the best information you have, but you can always just fail. Jeroen -- 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
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
Masking is often necessary, and information hiding is valuable, and depending on spooky action at a distance is generally foolish. But the granularity with which we want to track the propagation of a given error is *much* finer than the granularity of judicious information hiding: the error is likely to pass through a lot more stack frames than it is package boundaries, and the former transitions are the ones we need to track to properly diagnose errors [0], while the latter are the ones we need to mask to properly maintain global sanity [1]. To mask by default, at every annotation point, leads quickly to code whose action is characterised by bizarre and mystifying errors [2]. To *fail* to mask at sensible boundaries also leads to bugs, and we've seen one or two of those as well; the trouble is that masking is *always easy*, while pure annotation is (relatively) hard. To use a library that makes a point of keeping preservation comparatively verbose and complex is to fundamentally misunderstand the problem. Cheers William [0] it's not a perfect one-to-one match, but it's a reasonable heuristic. [1] it's not a perfect one-to-one match, but it's a reasonable heuristic. [2] this because developers are forced to choose between obscuring their logic with verbose workarounds for the masking... or to just avoid annotation entirely, and hence come to habitually avoid masking even when it's necessary. My lived experience indicates that they will overwhelmingly choose the latter, and that the consequences are unpretty. On Thu, May 29, 2014 at 9:16 PM, Nate Finch nate.fi...@canonical.comwrote: What Roger is talking about, is when an error is implementation-specific and may change if the implementation changes. For example, if you have a black box service that saves data, it might write to the network or it might write to a filesystem, but where it saves is an implementation detail. Let's say in the current implementation, Save() returns a FileNotFoundError if you haven't called Initialize() first, then people may write code that handles a FileNotFoundError by calling Initialize() now if you change the implementation to write to the network, so now if they haven't called Initialize(), you return a NetworkNotFoundError in the same situation their code will break. If you had instead written your own error, or just masked the type of the error to something generic, then they wouldn't have been able to write their code that makes bad assumptions. Note that this is the most simple case of the problem. In reality, it can be that some sub-sub-sub package was returning that specific error, and your code had no idea, it just passed it on up unchanged. This is where it gets insidious, because *your* code didn't change. Some dependency of a dependency changed that you might not even realize you were calling. And now people using your code are broken. If you masked all returned errors except the ones you explicitly wanted to return, then this couldn't happen. On Thu, May 29, 2014 at 3:00 PM, Jeroen Vermeulen jeroen.vermeu...@canonical.com wrote: On 2014-05-29 09:50, roger peppe wrote: On 29 May 2014 04:03, Tim Penhey tim.pen...@canonical.com wrote: Errors are worth treating specially here because they're they pass up through multiple layers, so it's very easy to break abstraction boundaries by relying on specific error types. I believe it's very important to *think* about the possible errors that can be returned from a given function, and not just throw up our hands and say you guys 5 layers down, why don't you just communicate *arbitrary stuff* to the guy 3 layers above. It may help to consider this as two problems. One: caller needs to know about a specific failure — e.g. because it's a failure to the callee but not to the caller. Definitely part of the contract. You either: (a) define a super-specific error (exception class, error code, etc.), or (b) document that standard error X means failure Y in this case, and the caller picks up the error as close as possible to its origin. With these errors you have to make sure that the information isn't diluted as it propagates, but usually you don't have to take it too far up the call chain. Two: a caller can deal better with some errors, given more detailed information. You can help by attaching more information to the error (tags, taxonomy, properties) but only on a best-effort basis. You accept that you don't know exactly which errors can come out of the code further down. For example, if you're writing code which speaks to another program over the network, you may want to know: is this connection still usable? Do I know what happened to the operation I requested? Were we able to connect in the first place? Am I doing something that shouldn't ever work, such as mis-spell a command? With these errors you act on the best information you have, but you can always just fail.
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
I just saw this thread after replying to the other one.
Fundamentally, I think comparing errors for equality
is misguided. That said, a simple approach that
doesn't use DeepEquals is possible:
func sameError(e1, e2 error) bool {
defer func() { recover() }()
return e1 == e2
}
On 28 May 2014 05:16, Tim Penhey tim.pen...@canonical.com wrote:
On 28/05/14 15:48, John Meinel wrote:
I think we need concrete examples which Tim should have in the test suite.
John
=:-
On May 28, 2014 6:50 AM, Andrew Wilkins andrew.wilk...@canonical.com
mailto:andrew.wilk...@canonical.com wrote:
On Wed, May 28, 2014 at 10:39 AM, John Meinel
j...@arbash-meinel.com mailto:j...@arbash-meinel.com wrote:
The address of the real value is the same.
Are you referring to the backing array? That is not what is being
compared, so that's not a useful property.
FWIW, replacing the icky unsafe bits with reflect.DeepEqual does work
but it just leaves me with an icky taste in my mouth.
Admittedly less icky than the unsafe work.
Tim
--
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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
I haven't seen any code or a description of what you're doing, so it's really hard for me to help. Broadly, what are you trying to accomplish? Maybe we can help avoid the need to do something that Go doesn't support well. On Tue, May 27, 2014 at 9:43 PM, Tim Penhey tim.pen...@canonical.comwrote: On 28/05/14 13:31, Nate Finch wrote: If you're talking about errgo, if all we care about is transition, but not what the actual types are, we could just record in the list when we mask the type. We could, but that would mean walking the list every time you want to check the cause. It would also change the meaning of cause, so a nil cause means get the cause from the previous (underlying) error. Do you not agree with me though that in the general Go language there should be a way to check identity? Or do you not agree? Tim On May 27, 2014 8:48 PM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. There seems to be no clean way to do this. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
I propose we change the name of the package to something other than errgo, so that Roger can keep his name, and we can make the behavior whatever fits best for Juju. Yes, we should make it a general-use package, but it should be designed for Juju, meeting our needs. Roger can post errgo wherever he wants and make it work exactly the way he wants it to work. I think that it's neither useful nor a good idea to write *and maintain* a package under the flag of Juju that we don't like the behavior of. I think the main problem with errgo as it exists, is that it is very awkward to allow error type information to pass through: return errgo.Mask(err, errgo.Any) This line is very misleading. It's saying - mask the error type, except wait, actually don't mask the type. It's never a good idea to use a function to do the opposite of what it's name says it does. Instead, it would be much better to have a method like this: errgo.MaintainErrorTypeAndAddTraceback(err) (with a better name, obviously, but that's not the point) For the record, I still find the API of errgo to be baffling without a long ramp up period. Having both an underlying error and a cause is hard to wrap my head around. I finally managed to load it all up out of long term storage, so I get it again... but it's not a terribly easy package to understand, in my opinion. I don't think there's any need to debate passing type information through or not... that's something we can do with any package we're considering. I think we're agreed that we should maintain the current behavior with the new package just adding tracebacks, and then we can slowly introduce behavior changes as we decide we need them. -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
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
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
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.comwrote: 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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. There seems to be no clean way to do this. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On 28/05/14 13:31, Nate Finch wrote: If you're talking about errgo, if all we care about is transition, but not what the actual types are, we could just record in the list when we mask the type. We could, but that would mean walking the list every time you want to check the cause. It would also change the meaning of cause, so a nil cause means get the cause from the previous (underlying) error. Do you not agree with me though that in the general Go language there should be a way to check identity? Or do you not agree? Tim On May 27, 2014 8:48 PM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. There seems to be no clean way to do this. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On Wed, May 28, 2014 at 8:47 AM, Tim Penhey tim.pen...@canonical.comwrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. Can you explain where equality fails? I guess you're thinking you'd like to do something like x is y in Python. There's no such thing as objects in Go, so no universal definition of identity either. There seems to be no clean way to do this. Tim -- 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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On 28/05/14 13:48, Andrew Wilkins wrote: On Wed, May 28, 2014 at 8:47 AM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. Can you explain where equality fails? Equality fails when the interface is satisfied by a non-comparable value type, like a struct with a slice in it. I guess you're thinking you'd like to do something like x is y in Python. There's no such thing as objects in Go, so no universal definition of identity either. But an interface is effectively two pointers, one to the type and one to the thing that satisfies the interface. Identity in that case is pretty simple. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On Wed, May 28, 2014 at 9:52 AM, Tim Penhey tim.pen...@canonical.comwrote: On 28/05/14 13:48, Andrew Wilkins wrote: On Wed, May 28, 2014 at 8:47 AM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. Can you explain where equality fails? Equality fails when the interface is satisfied by a non-comparable value type, like a struct with a slice in it. I see. In that case, I guess you'd have to use reflect.DeepEquals to collapse arbitrary errors. I guess you're thinking you'd like to do something like x is y in Python. There's no such thing as objects in Go, so no universal definition of identity either. But an interface is effectively two pointers, one to the type and one to the thing that satisfies the interface. Identity in that case is pretty simple. If the pointer is the same, which it won't be if you're storing a slice in an interface; slices are wider than pointers. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On Wed, May 28, 2014 at 10:39 AM, John Meinel j...@arbash-meinel.comwrote: The address of the real value is the same. Are you referring to the backing array? That is not what is being compared, so that's not a useful property. John =:- On May 28, 2014 6:04 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, May 28, 2014 at 9:52 AM, Tim Penhey tim.pen...@canonical.comwrote: On 28/05/14 13:48, Andrew Wilkins wrote: On Wed, May 28, 2014 at 8:47 AM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. Can you explain where equality fails? Equality fails when the interface is satisfied by a non-comparable value type, like a struct with a slice in it. I see. In that case, I guess you'd have to use reflect.DeepEquals to collapse arbitrary errors. I guess you're thinking you'd like to do something like x is y in Python. There's no such thing as objects in Go, so no universal definition of identity either. But an interface is effectively two pointers, one to the type and one to the thing that satisfies the interface. Identity in that case is pretty simple. If the pointer is the same, which it won't be if you're storing a slice in an interface; slices are wider than pointers. -- 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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
I think we need concrete examples which Tim should have in the test suite. John =:- On May 28, 2014 6:50 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, May 28, 2014 at 10:39 AM, John Meinel j...@arbash-meinel.comwrote: The address of the real value is the same. Are you referring to the backing array? That is not what is being compared, so that's not a useful property. John =:- On May 28, 2014 6:04 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, May 28, 2014 at 9:52 AM, Tim Penhey tim.pen...@canonical.comwrote: On 28/05/14 13:48, Andrew Wilkins wrote: On Wed, May 28, 2014 at 8:47 AM, Tim Penhey tim.pen...@canonical.com mailto:tim.pen...@canonical.com wrote: On 28/05/14 12:43, Nate Finch wrote: This sounds like one of those if you have to ask this question, you're doing something wrong. Can you give an example of where we need this? Sure... let's say we have a stack of errors, for simplicity of the argument lets say it is a slice of error interface values. stack []error * an error is pushed on to the stack initially, we now have one error * the same error is pushed (or appended - I don't care) * we now have the same error twice * I push a new error on the stack, so it looks a little like this [err1, err1, err2] right? Now iterating through this slice I want to know when the error changes. Can you explain where equality fails? Equality fails when the interface is satisfied by a non-comparable value type, like a struct with a slice in it. I see. In that case, I guess you'd have to use reflect.DeepEquals to collapse arbitrary errors. I guess you're thinking you'd like to do something like x is y in Python. There's no such thing as objects in Go, so no universal definition of identity either. But an interface is effectively two pointers, one to the type and one to the thing that satisfies the interface. Identity in that case is pretty simple. If the pointer is the same, which it won't be if you're storing a slice in an interface; slices are wider than pointers. -- 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
Re: interface identity (was Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo))
On 28/05/14 15:48, John Meinel wrote: I think we need concrete examples which Tim should have in the test suite. John =:- On May 28, 2014 6:50 AM, Andrew Wilkins andrew.wilk...@canonical.com mailto:andrew.wilk...@canonical.com wrote: On Wed, May 28, 2014 at 10:39 AM, John Meinel j...@arbash-meinel.com mailto:j...@arbash-meinel.com wrote: The address of the real value is the same. Are you referring to the backing array? That is not what is being compared, so that's not a useful property. FWIW, replacing the icky unsafe bits with reflect.DeepEqual does work but it just leaves me with an icky taste in my mouth. Admittedly less icky than the unsafe work. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 14 May 2014 10:24, Tim Penhey tim.pen...@canonical.com wrote: Hi all, I took it upon myself to get Rog's errgo library used inside juju-core. Thanks for doing this. Dimiter recently did a hunk of work in the juju-core/errors package to have functions to add context to some core error types while keeping their type. What I did was to move this errors package out of juju-core and into its own package, and update it to use the errgo as a base. For those that are interested the diff for the errors_test (to show that everything works as before) is as shown at the bottom of this email. I also added in simple methods based on my earlier error handling package, but changed them to be based on errgo. This adds in the functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. 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. Since it's likely that other packages will start to use errgo, and they don't necessarily want to be importing juju/errors, there are advantages to using the same error generation and classification style that any other errgo-using package would use - in particular, it makes the code easier to manipulate programmatically. There are also advantages in not having two subtly different APIs around, even if one is implemented in terms of the other. I believe we should pick one set of calls and use them consistently throughout the code base. BTW, I have a gofix clone which automates 95+% of the changes to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix It splits the changes into several modules so that the changes can be made in more-easily verifiable steps. I have tested juju/errors with both gc and gccgo and the tests pass with both. Interestingly juju/errgo has a few test failures with gccgo, but Dave is looking at them and assures me they are simple fixes. As an aside, I do think that any new dependencies we add should pass all tests with both gc and gccgo. Thanks a lot for doing this. I really should install and try gccgo! cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
I agree with Roger, I don't think a wrapper around Errgo is the right way to go. If we don't like the way errgo behaves, we should change it. It's our package, let's have it work the way we want it to work. On Thu, May 22, 2014 at 10:47 AM, roger peppe rogpe...@gmail.com wrote: On 14 May 2014 10:24, Tim Penhey tim.pen...@canonical.com wrote: Hi all, I took it upon myself to get Rog's errgo library used inside juju-core. Thanks for doing this. Dimiter recently did a hunk of work in the juju-core/errors package to have functions to add context to some core error types while keeping their type. What I did was to move this errors package out of juju-core and into its own package, and update it to use the errgo as a base. For those that are interested the diff for the errors_test (to show that everything works as before) is as shown at the bottom of this email. I also added in simple methods based on my earlier error handling package, but changed them to be based on errgo. This adds in the functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. 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. Since it's likely that other packages will start to use errgo, and they don't necessarily want to be importing juju/errors, there are advantages to using the same error generation and classification style that any other errgo-using package would use - in particular, it makes the code easier to manipulate programmatically. There are also advantages in not having two subtly different APIs around, even if one is implemented in terms of the other. I believe we should pick one set of calls and use them consistently throughout the code base. BTW, I have a gofix clone which automates 95+% of the changes to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix It splits the changes into several modules so that the changes can be made in more-easily verifiable steps. I have tested juju/errors with both gc and gccgo and the tests pass with both. Interestingly juju/errgo has a few test failures with gccgo, but Dave is looking at them and assures me they are simple fixes. As an aside, I do think that any new dependencies we add should pass all tests with both gc and gccgo. Thanks a lot for doing this. I really should install and try gccgo! 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
Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
Hi all,
I took it upon myself to get Rog's errgo library used inside juju-core.
Dimiter recently did a hunk of work in the juju-core/errors package to
have functions to add context to some core error types while keeping
their type.
What I did was to move this errors package out of juju-core and into its
own package, and update it to use the errgo as a base. For those that
are interested the diff for the errors_test (to show that everything
works as before) is as shown at the bottom of this email.
I also added in simple methods based on my earlier error handling
package, but changed them to be based on errgo. This adds in the
functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack.
One key difference in behaviour between these methods and the base errgo
ones is that the cause is always passed through. Unless you call Wrap,
in which case the cause changes, but that is the purpose of the call.
I have a branch of juju-core up as work in progress that does the following:
* removes the dependency on github.com/errgo/errgo
* changes uses of errgo/errgo to juju/errors
* adds both juju/errgo and juju/errors to the dependency file
* removes the juju-core/errors package
* changes all the juju-core/errors import statement to use
github.com/juju/errors (and puts the import statement in the
right block)
https://code.launchpad.net/~thumper/juju-core/juju-errors/+merge/219311
3602 lines (+252/-658) 156 files modified
I have tested juju/errors with both gc and gccgo and the tests pass with
both. Interestingly juju/errgo has a few test failures with gccgo, but
Dave is looking at them and assures me they are simple fixes.
As an aside, I do think that any new dependencies we add should pass all
tests with both gc and gccgo.
Cheers,
Tim
$ diff -u ~/src/juju-core/trunk/errors/errors_test.go errors_test.go
--- /home/tim/src/juju-core/trunk/errors/errors_test.go 2014-05-14
19:18:53.376121000 +1200
+++ errors_test.go 2014-05-13 17:14:50.329051637 +1200
@@ -1,5 +1,5 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
+// Copyright 2013, 2014 Canonical Ltd.
+// Licensed under the GPLv3, see LICENCE file for details.
package errors_test
@@ -8,12 +8,10 @@
fmt
reflect
runtime
- testing
+ github.com/juju/errors
jc github.com/juju/testing/checkers
gc launchpad.net/gocheck
-
- launchpad.net/juju-core/errors
)
// errorInfo holds information about a single error type: a satisfier
@@ -40,10 +38,6 @@
var _ = gc.Suite(errorsSuite{})
-func Test(t *testing.T) {
- gc.TestingT(t)
-}
-
func (t *errorInfo) satisfierName() string {
value := reflect.ValueOf(t.satisfier)
f := runtime.FuncForPC(value.Pointer())
@@ -175,6 +169,12 @@
runErrorTests(c, errorTests, true)
}
+func (*errorsSuite) TestContextfNotNewer(c *gc.C) {
+ err := fmt.Errorf(simple error)
+ errors.Contextf(err, annotate)
+ c.Assert(err, gc.ErrorMatches, annotate: simple error)
+}
+
func (*errorsSuite) TestAllErrors(c *gc.C) {
errorTests := []errorTest{}
for _, errInfo := range allErrors {
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
I'd like gccgo to pass reliably, but I think it falls under a CI test
rather than a pre-commit test. (Because otherwise it roughly doubles the
time it takes to know whether your patch will land.)
I don't have specific feedback on the behavior differences, but the changes
seem sane to me. An errors library is certainly something that I'd like to
see shared between code bases.
John
=:-
On Wed, May 14, 2014 at 1:24 PM, Tim Penhey tim.pen...@canonical.comwrote:
Hi all,
I took it upon myself to get Rog's errgo library used inside juju-core.
Dimiter recently did a hunk of work in the juju-core/errors package to
have functions to add context to some core error types while keeping
their type.
What I did was to move this errors package out of juju-core and into its
own package, and update it to use the errgo as a base. For those that
are interested the diff for the errors_test (to show that everything
works as before) is as shown at the bottom of this email.
I also added in simple methods based on my earlier error handling
package, but changed them to be based on errgo. This adds in the
functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap,
ErrorStack.
One key difference in behaviour between these methods and the base errgo
ones is that the cause is always passed through. Unless you call Wrap,
in which case the cause changes, but that is the purpose of the call.
I have a branch of juju-core up as work in progress that does the
following:
* removes the dependency on github.com/errgo/errgo
* changes uses of errgo/errgo to juju/errors
* adds both juju/errgo and juju/errors to the dependency file
* removes the juju-core/errors package
* changes all the juju-core/errors import statement to use
github.com/juju/errors (and puts the import statement in the
right block)
https://code.launchpad.net/~thumper/juju-core/juju-errors/+merge/219311
3602 lines (+252/-658) 156 files modified
I have tested juju/errors with both gc and gccgo and the tests pass with
both. Interestingly juju/errgo has a few test failures with gccgo, but
Dave is looking at them and assures me they are simple fixes.
As an aside, I do think that any new dependencies we add should pass all
tests with both gc and gccgo.
Cheers,
Tim
$ diff -u ~/src/juju-core/trunk/errors/errors_test.go errors_test.go
--- /home/tim/src/juju-core/trunk/errors/errors_test.go 2014-05-14
19:18:53.376121000 +1200
+++ errors_test.go 2014-05-13 17:14:50.329051637 +1200
@@ -1,5 +1,5 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
+// Copyright 2013, 2014 Canonical Ltd.
+// Licensed under the GPLv3, see LICENCE file for details.
package errors_test
@@ -8,12 +8,10 @@
fmt
reflect
runtime
- testing
+ github.com/juju/errors
jc github.com/juju/testing/checkers
gc launchpad.net/gocheck
-
- launchpad.net/juju-core/errors
)
// errorInfo holds information about a single error type: a satisfier
@@ -40,10 +38,6 @@
var _ = gc.Suite(errorsSuite{})
-func Test(t *testing.T) {
- gc.TestingT(t)
-}
-
func (t *errorInfo) satisfierName() string {
value := reflect.ValueOf(t.satisfier)
f := runtime.FuncForPC(value.Pointer())
@@ -175,6 +169,12 @@
runErrorTests(c, errorTests, true)
}
+func (*errorsSuite) TestContextfNotNewer(c *gc.C) {
+ err := fmt.Errorf(simple error)
+ errors.Contextf(err, annotate)
+ c.Assert(err, gc.ErrorMatches, annotate: simple error)
+}
+
func (*errorsSuite) TestAllErrors(c *gc.C) {
errorTests := []errorTest{}
for _, errInfo := range allErrors {
--
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
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On Wed, May 14, 2014 at 8:12 AM, John Meinel j...@arbash-meinel.com wrote: I'd like gccgo to pass reliably, but I think it falls under a CI test rather than a pre-commit test. (Because otherwise it roughly doubles the time it takes to know whether your patch will land.) I agree. We can setup gccgo-based test runs for libraries on amd64 and ppc. I am not certain about priorities. Is getting juju into github more important? On Wed, May 14, 2014 at 1:24 PM, Tim Penhey tim.pen...@canonical.com wrote: I took it upon myself to get Rog's errgo library used inside juju-core. I have a branch of juju-core up as work in progress that does the following: * removes the dependency on github.com/errgo/errgo * changes uses of errgo/errgo to juju/errors * adds both juju/errgo and juju/errors to the dependency file * removes the juju-core/errors package * changes all the juju-core/errors import statement to use github.com/juju/errors (and puts the import statement in the right block) I predict 1.18 will fail in CI. because it will still want errgo, which go-get did will not get. Something similar happened with when 1.17.x dropped a dep that 1.16.x wanted. I will add a shim to keep 1.18 building. -- Curtis Hovey Canonical Cloud Development and Operations http://launchpad.net/~sinzui -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
