Code review comment for lp:~rvb/gwacl/not-found-errors

Revision history for this message
Gavin Panella (allenap) wrote :

> > Looks okay, but I want you to consider [1] before I approve.
>
> This looks much too big to be considered *before* this lands to be honest.

Yeah, I guess it's *instead* of parts of this branch. Things like
IsNotFoundError can be reimplemented to use this quite easily.

> An
> interesting idea but clearly outside the scope of this branch; the good thing
> about this branch is that all the extendError()/IsNotFoundError() stuff is
> well encapsulated so we could move to something like what you describe in the
> future.

I'm not sure it's clearly outside of scope, but okay.

> A few things about the error chain idea:
> - I like the general idea although I'm afraid that it might be flagged as non-
> idiomatic (whatever that means) because it's basically a slightly awkward
> implementation of a dynamic traceback being built manually :).

We're the GWACL maintainers, so we get to say what's idiomatic. If
smooshing strings together for errors is truly what the Go mainstream
call idiomatic then I'm happy to be a freak. I don't think it is
though. They like tracebacks too, they're just having some cognitive
dissonance issues between tracebacks with panic (or what a duck would
call exceptions) and the dogmatic checking and passing of errors by
hand.

> - Such a generic pattern, if a complete analysis prove that it's worth it,
> should probably live in its own project so that many projects can benefit from
> it.

It's only 39 lines ;)

It might be generically useful, but everything has to start somewhere.
Putting it in a sub-package or just a new file is enough to get going.

review: Approve

« Back to merge proposal