|
|
Created:
12 years, 1 month ago by niemeyer Modified:
12 years, 1 month ago Reviewers:
mp+95183 Visibility:
Public. |
Description
https://code.launchpad.net/~niemeyer/juju/store-publish-tips/+merge/95183
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 8
Patch Set 2 : store: add PublishCharmsDistro function #
MessagesTotal messages: 17
Please take a look.
Sign in to reply to this message.
LGTM (also, very instructive; thanks :))
Sign in to reply to this message.
LGTM although i won't pretend to understand all the test logic. https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err i wonder if these error messages should have some context. for example, if i call PublishCharmsDistro and i get the error 'map is missing "value" field' then i'll struggle to realise that it's come from a call to BranchTips and not Login or Distro. https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode113 store/lpad.go:113: if len(u) < 5 || u[1] != "charms" || u[4] != "trunk" || len(u[0]) < 0 || u[0][0] != '~' { s/len(u[0]) < 0/u[0] == ""/ or len(u[0]) == 0 the logic here isn't so easy to read. it's possible that it might be clearer to use a regexp. YMMV. (.*)(~[^/]+)/charms/([^/]+)/([^/]+)/trunk
Sign in to reply to this message.
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 16:54:50, rog wrote: > i wonder if these error messages should have some context. for example, if i > call PublishCharmsDistro and i get the error 'map is missing "value" field' then Agreed, that's not a good error message, but this isn't the place to fix it. Let's make sure lpad returns more reasonable errors so that we can tell where they're coming from. https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode113 store/lpad.go:113: if len(u) < 5 || u[1] != "charms" || u[4] != "trunk" || len(u[0]) < 0 || u[0][0] != '~' { On 2012/02/29 16:54:50, rog wrote: > s/len(u[0]) < 0/u[0] == ""/ > or len(u[0]) == 0 > > the logic here isn't so easy to read. it's possible that it might be clearer to > use a regexp. YMMV. > > (.*)(~[^/]+)/charms/([^/]+)/([^/]+)/trunk The if statement feels trivial if compared to parsing that expression in my head for correctness, and it seems that you agree even without realizing. That expression isn't right. The documentation for the function also explains what is the expected URL format, so if you read it the logic should be obvious.
Sign in to reply to this message.
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 17:27:35, niemeyer wrote: > On 2012/02/29 16:54:50, rog wrote: > > i wonder if these error messages should have some context. for example, if i > > call PublishCharmsDistro and i get the error 'map is missing "value" field' > then > > Agreed, that's not a good error message, but this isn't the place to fix it. > Let's make sure lpad returns more reasonable errors so that we can tell where > they're coming from. this is a philosophical issue that it would be useful to resolve. i'm not sure that every error message should describe the function that it's erroring from. i don't mind that, for instance, many functions return io.EOF with no added information - the caller is always in a good position to add information depending on the context of the call, and i'd be happy to see that context added here. if we want to use the "error describes the function the error occurred in" approach, then we should add a context here anyway, to say that the error is from PublishCharmsDistro. it's better, i think to describe what we were trying to do when the error occurred. https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode113 store/lpad.go:113: if len(u) < 5 || u[1] != "charms" || u[4] != "trunk" || len(u[0]) < 0 || u[0][0] != '~' { On 2012/02/29 17:27:35, niemeyer wrote: > On 2012/02/29 16:54:50, rog wrote: > > s/len(u[0]) < 0/u[0] == ""/ > > or len(u[0]) == 0 > > > > the logic here isn't so easy to read. it's possible that it might be clearer > to > > use a regexp. YMMV. > > > > (.*)(~[^/]+)/charms/([^/]+)/([^/]+)/trunk > > The if statement feels trivial if compared to parsing that expression in my head > for correctness, and it seems that you agree even without realizing. That > expression isn't right. > > The documentation for the function also explains what is the expected URL > format, so if you read it the logic should be obvious. i was confused slightly by the fact that the prefix must end with a slash. *shrug*. neither way is great; it's a pity the testing logic obscures the simple normal logic. BTW how *is* that expression wrong? i just checked it and it seems to work ok.
Sign in to reply to this message.
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 17:57:23, rog wrote: > this is a philosophical issue that it would be useful to resolve. Sounds good. > i'm not sure that every error message should describe the function that it's > erroring from. Agreed. That's my point too. As far as I understand, though, you're asking to make PublishCharmsDistro do exactly that, on behalf of each of the functions from lpad. I'm not keen on that either, since you're merely shifting who's reporting the problem. > i don't mind that, for instance, many functions return io.EOF with no added > information EOF in many cases should have information added since the call site may not be in a good position to tell what failed. Both ReadMeta and ReadConfig enrich such errors, without simply adding a "FunctionFoo:" prefix. Do you disagree with that? > - the caller is always in a good position to add information > depending on the context of the call, and i'd be happy to see that context added > here. The error message you mentioned is lpad itself making things harder than they should be. I'm not keen on fixing lpad's bugs here. I'm not suggesting we simply add a "FunctionFoo:" prefix, though. > if we want to use the "error describes the function the error occurred in" No, I don't think doing that blindly is appropriate. The standard library doesn't do that, juju doesn't do that, and I see no reason for us to start doing that kind of error mangling blindly now. I'm happy to fix problems, though. The one you mentioned is a bug in lpad. You mentioned problems in gozk before, and they must be fixed too. If you see other problems, let's talk about them to make sure we get reasonable error behavior. https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode113 store/lpad.go:113: if len(u) < 5 || u[1] != "charms" || u[4] != "trunk" || len(u[0]) < 0 || u[0][0] != '~' { On 2012/02/29 17:57:23, rog wrote: > BTW how *is* that expression wrong? i just checked it and it seems to work ok. That's the point. It's easy to screw up with regular expressions. (.*)~ matches "foo~" too.
Sign in to reply to this message.
On 29 February 2012 18:13, <n13m3y3r@gmail.com> wrote: > > https://codereview.appspot.com/5704056/diff/1/store/lpad.go > File store/lpad.go (right): > > https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 > store/lpad.go:50: return err > On 2012/02/29 17:57:23, rog wrote: >> >> this is a philosophical issue that it would be useful to resolve. > > > Sounds good. > > >> i'm not sure that every error message should describe the function > > that it's >> >> erroring from. > > > Agreed. That's my point too. As far as I understand, though, you're > asking to make PublishCharmsDistro do exactly that, on behalf of each of > the functions from lpad. I'm not keen on that either, since you're > merely shifting who's reporting the problem. if PublishCharmsDistro returns an error, it's useful for the caller to know where that error occurred. i think we both agree on that. if it returns, for example, an "invalid URL escape" error (which is possible currently), it's not clear to the caller where that error happened. if instead, it returns an error that says: "cannot log in to launchpad: invalid URL escape" then the source of the problem is quickly narrowed down, even in the face of an error that doesn't identify itself. this is my proposal: if there are several places within a function that can fail, that we identify in the returned error message which of those places has failed. sometimes the error itself is enough (for instance os.PathError quite often suffices); when it doesn't, i think we should go with the above proposal. >> i don't mind that, for instance, many functions return io.EOF with no >> added nformation > > EOF in many cases should have information added since the call site may > not be in a good position to tell what failed. Both ReadMeta and > ReadConfig enrich such errors, without simply adding a "FunctionFoo:" > prefix. Do you disagree with that? not at all. that's what i'm suggesting, in fact. we should always be thinking "does this error make sense to the caller?" an error that makes sense to the caller of, say lpad.Login, will not necessarily make sense to the caller of PublishCharmsDistro. the place that the error changes context is in the return statement from PublishCharmsDistro, which is why i think that's a perfect place to add context. > I'm happy to fix problems, though. The one you mentioned is a bug in > lpad. You mentioned problems in gozk before, and they must be fixed too. > If you see other problems, let's talk about them to make sure we get > reasonable error behavior. i can mention any number of other such "bugs". i don't think they're bugs as such (well, perhaps the one i mentioned is, as it doesn't even really make sense to the caller of BranchTips; see my proposal above); just that the error information needs appropriate context > That's the point. It's easy to screw up with regular expressions. (.*)~ > matches "foo~" too. that was deliberate - it matches your documentation for the function.
Sign in to reply to this message.
> if PublishCharmsDistro returns an error, it's useful for the caller > to know where that error occurred. i think we both agree on that. Indeed. > if it returns, for example, an "invalid URL escape" error (which > is possible currently), it's not clear to the caller where that error happened. > > if instead, it returns an error that says: "cannot log in to > launchpad: invalid URL escape" So you mean "PublishDistroCharms" is a better place to have an error message saying "cannot login into Launchpad" than lpad.Login? Sorry, but that's not acceptable. > this is my proposal: if there are several places within a function > that can fail, that we identify in the returned error message which > of those places has failed. I disagree in having that as a general base line, as this would mean a wrapper for every single error of every single function. We should enrich the error message when there's something valuable to be said in such a context. Not the case here. > sometimes the error itself is enough (for instance os.PathError quite > often suffices); when it doesn't, i think we should go with the abov fe > proposal. Exactly. PathError is quite often enough because the message is relevant. That's what we should try to achieve. >> That's the point. It's easy to screw up with regular expressions. (.*)~ >> matches "foo~" too. > > that was deliberate - it matches your documentation for the function. Only if you have no idea about what ~user means, which would make me a bit worried. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 29 February 2012 19:24, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> if PublishCharmsDistro returns an error, it's useful for the caller >> to know where that error occurred. i think we both agree on that. > > Indeed. > >> if it returns, for example, an "invalid URL escape" error (which >> is possible currently), it's not clear to the caller where that error happened. >> >> if instead, it returns an error that says: "cannot log in to >> launchpad: invalid URL escape" > > So you mean "PublishDistroCharms" is a better place to have an error > message saying "cannot login into Launchpad" than lpad.Login? Sorry, > but that's not acceptable. The way I look at it is that *any* error returned from lpad.Login means "cannot log in to Launchpad". This is what I meant by identifying the function within the error message. If we went that way, then every error return from lpad.Login would be of the form: fmt.Errorf("cannot log in to launchpad: %v", err) I don't think that's a good way to go - it means we're writing the same redundant error message several times. In my view "cannot log in to launchpad" is the *context* for the error; "invalid URL escape" is the error itself. By adding appropriate context to each error return, the error can identify the major failure modes of the function ("this is what we were trying to do: it failed for this reason") When we know something about a particular error message, we might choose to return it unannotated if we know that it identifies what we were trying to do as well as the error itself. os.PathError can fall into this category, for example; or when a function has only one major failure mode. >> this is my proposal: if there are several places within a function >> that can fail, that we identify in the returned error message which >> of those places has failed. > > I disagree in having that as a general base line, as this would mean a > wrapper for every single error of every single function. We would certainly need many more wrappers than we use today, yes. > We should > enrich the error message when there's something valuable to be said in > such a context. Not the case here. As I said above, there is valuable information that's added here: by doing this we make it possible to quickly identify what has gone wrong from the error message. I have found this style of error reporting to be very useful in practice. By using it, I think we could greatly improve the juju diagnostics. I think the small code penalty is worth paying.
Sign in to reply to this message.
>> So you mean "PublishDistroCharms" is a better place to have an error >> message saying "cannot login into Launchpad" than lpad.Login? Sorry, >> but that's not acceptable. > > The way I look at it is that *any* error returned from lpad.Login means > "cannot log in to Launchpad". Yes, that's exactly the problem. *any* error, *ever* returned from lpad.Login, is "launchpad login" failing. > This is what I meant by identifying > the function within the error message. If we went that way, then > every error return from lpad.Login would be of the form: > fmt.Errorf("cannot log in to launchpad: %v", err) Yep, or more realistically, &SomeError{"launchpad login", err} > I don't think that's a good way to go - it means we're writing the same > redundant error message several times. I find it curious that you're not happy with lpad.Login telling you that there's a problem with lpad.Login, but you're happy with every single call site that ever calls lpad.Login to say that there's a problem with lpad.Login. (...) > When we know something about a particular error message, > we might choose to return it unannotated if we know that > it identifies what we were trying to do as well as the error > itself. os.PathError can fall into this category, for example; > or when a function has only one major failure mode. PathError falls into that category because it is providing the context by itself, exactly as I'm saying lpad.Login should. > As I said above, there is valuable information that's added here: by > doing this we make it possible to quickly identify what has gone wrong > from the error message. Both ways allow identifying what went wrong. > I have found this style of error reporting to be very useful in practice. > By using it, I think we could greatly improve the juju diagnostics. > I think the small code penalty is worth paying. We can have the cake and eat it too. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 1 March 2012 13:35, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >>> So you mean "PublishDistroCharms" is a better place to have an error >>> message saying "cannot login into Launchpad" than lpad.Login? Sorry, >>> but that's not acceptable. >> >> The way I look at it is that *any* error returned from lpad.Login means >> "cannot log in to Launchpad". > > Yes, that's exactly the problem. *any* error, *ever* returned from > lpad.Login, is "launchpad login" failing. i disagree. it depends on the context. sometimes it might be "log in to second launchpad account: some error" or "log in to launchpad, 5th try: some error". by putting the function identification in the error that's returned, we're trying to second-guess what kind of error will be appropriate for the caller to return. i'm saying we don't need to guess; we can let the caller do it. >> This is what I meant by identifying >> the function within the error message. If we went that way, then >> every error return from lpad.Login would be of the form: >> fmt.Errorf("cannot log in to launchpad: %v", err) > > Yep, or more realistically, &SomeError{"launchpad login", err} if you go this route (and i agree it's a possibility), then PublishCharmsDistro should be written like this: func PublishCharmsDistro(store *Store, apiBase lpad.APIBase) error { oauth := &lpad.OAuth{Anonymous: true, Consumer: "juju"} root, err := lpad.Login(apiBase, oauth) if err != nil { return fmt.Errorf("publishing charms distro: %v", err) } distro, err := root.Distro("charms") if err != nil { return fmt.Errorf("publishing charms distro: %v", err) } tips, err := distro.BranchTips(time.Time{}) if err != nil { return fmt.Errorf("publishing charms distro: %v", err) } [...] or every function and/or package should get its own error type. i'd prefer not to do that. >> I don't think that's a good way to go - it means we're writing the same >> redundant error message several times. > > I find it curious that you're not happy with lpad.Login telling you > that there's a problem with lpad.Login, but you're happy with every > single call site that ever calls lpad.Login to say that there's a > problem with lpad.Login. i don't see it this way. i see it as the function describing what it was trying to do in a way that makes sense to its caller. by doing things this way, we ensure that even if a package *does* return bare errors, the error that's presented to the user has useful context. >> I have found this style of error reporting to be very useful in practice. >> By using it, I think we could greatly improve the juju diagnostics. >> I think the small code penalty is worth paying. > > We can have the cake and eat it too. i'm not sure.
Sign in to reply to this message.
> i disagree. it depends on the context. > sometimes it might be "log in to second launchpad account: some error" > or "log in to launchpad, 5th try: some error". "On second account: launchpad login: some error" "Failed after 5th try: launchpad login: some error" > i'm saying we don't need to guess; we can let the caller do it. No guessing being done. > if you go this route (and i agree it's a possibility), then PublishCharmsDistro > should be written like this: As I said in this thread already, it's not reasonable to be mangling messages blindly just because. If there isn't a problem, we don't need to solve it. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 1 March 2012 14:31, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> i disagree. it depends on the context. >> sometimes it might be "log in to second launchpad account: some error" >> or "log in to launchpad, 5th try: some error". > > "On second account: launchpad login: some error" > "Failed after 5th try: launchpad login: some error" > >> i'm saying we don't need to guess; we can let the caller do it. > > No guessing being done. > >> if you go this route (and i agree it's a possibility), then PublishCharmsDistro >> should be written like this: > > As I said in this thread already, it's not reasonable to be mangling > messages blindly just because. If there isn't a problem, we don't need > to solve it. why should lpad.Login return a "launchpad login:" error when PublishCharmsDistro should not return a "publish charms:" error? it seems that you're deciding on the appropriate error message depending on who's calling the function - that seems wrong to me, and in the larger context it's unsustainable because you won't know what callers you have.
Sign in to reply to this message.
> why should lpad.Login return a "launchpad login:" error > when PublishCharmsDistro should not return a "publish charms:" error? Where did you have problems with the error messages returned by this PublishCharmsDistro? When was it confusing? -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 1 March 2012 15:47, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> why should lpad.Login return a "launchpad login:" error >> when PublishCharmsDistro should not return a "publish charms:" error? > > Where did you have problems with the error messages returned by this > PublishCharmsDistro? When was it confusing? any time i have a function like this: func Foo() error { err = something(...) if err != nil { return err } err := store.PublishCharmsDistro(...) if err != nil { return err } err = somethingElse(...) if err != nil { return err } return nil } then the return value from Foo will be confusing, because it won't be obvious from the error message which thing has failed. you're suggesting, i think, that if i do encounter this situation, that i go and fix PublishCharmsDistro so that it identifies the error (rather than returning "cannot publish charms: %v" within Foo). and that's what i'm talking about - this is changing the error message based on who is calling it, rather than getting the error message right to start with, for all time.
Sign in to reply to this message.
On Thu, Mar 1, 2012 at 13:12, roger peppe <rogpeppe@gmail.com> wrote: >> Where did you have problems with the error messages returned by this >> PublishCharmsDistro? When was it confusing? > > any time i have a function like this: You're not answering my question. We're not talking about "any time" here. We have two particular cases at hand, and I've explained why it's sensible for lpad.Login to report "launchpad login", and why there's no point in PublishCharmDistro to do so. This means we have different cases, which require consideration on their own, and different solutions. That's how the standard library is structured as well. If you find other errors that need consideration, we can talk about them. No wild-goose chasing, otherwise. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
*** Submitted: store: add PublishCharmsDistro function R= CC= https://codereview.appspot.com/5704056
Sign in to reply to this message.
|