store: add PublishCharmsDistro function

https://code.launchpad.net/~niemeyer/juju/store-publish-tips/+merge/95183

(do not edit description out of merge proposal)

[niemeyer, 2012-02-29 14:53:28]

Please take a look.

[fwereade, 2012-02-29 15:24:20]

LGTM (also, very instructive; thanks :))

[rog, 2012-02-29 16:54:50]

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

[niemeyer, 2012-02-29 17:27:34]

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.

[rog, 2012-02-29 17:57:23]

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.

[niemeyer, 2012-02-29 18:13:56]

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.

[rog, 2012-02-29 18:46:37]

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.

[gustavo_niemeyer.net, 2012-02-29 19:24:38]

> 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.

[rog, 2012-03-01 09:06:06]

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.

[gustavo_niemeyer.net, 2012-03-01 13:36:20]

>> 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.

[rog, 2012-03-01 13:58:12]

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.

[gustavo_niemeyer.net, 2012-03-01 14:31:53]

> 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.

[rog, 2012-03-01 15:22:40]

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.

[gustavo_niemeyer.net, 2012-03-01 15:47:51]

> 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.

[rog, 2012-03-01 16:12:46]

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.

[gustavo_niemeyer.net, 2012-03-01 16:19:41]

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.

[niemeyer, 2012-03-01 20:36:45]

*** Submitted:

store: add PublishCharmsDistro function

R=
CC=
https://codereview.appspot.com/5704056