Hey John,
Interesting mail.
On 07/11/2007, at 7:04 AM, John Casey wrote:
The point of this exercise is to improve the readability of the
code, and allow someone browsing through the source to know what
the potential exception chains actually are, without requiring a
full static analysis of maven's entire codebase. It has the added
benefit of forcing us to look at these exception chains a little
bit, and figure out where we might be able to consolidate exception
classes, subclass exceptions, and attach better context information
at a given exception's construction.
+1 :)
In the case of the ProjectBuildingException, I believe the
profusion of constructors is bad;
I'm not quite sure why this is a problem? It is good to centralise
any such conversion (if it is necessary), and the important thing is
that the exception only contains one type so it is easy to represent
it consistently.
The other thing I've been struggling with lately is the use of
java.net.URI internally within the ProjectBuildingException. I've
done static analysis of maven's core, and the URI instance is not
used intact anywhere in the code. While, taken with the super-POM's
location, this field type might make a sort of pure sense, I don't
see the value of incurring additional instability here. We're
converting pom locations from (and, supposedly, to) java.io.File
instances using information in this exception, but with the new
changes, we're adding all sorts of new complexity in the code by
constructing File instances so we can call toUri() on them...which
in some cases, causes the build to tank because the file path has a
space in it.
This last sentence sounds back to front - it was the URL that caused
the problems, file conversions are fine? The toURI Javadoc in fact
says that it is designed to ensure things get encoded properly from
files :) The URL problem simply seems a bug in 1.4 that was fixed in
1.5 and we have a workaround in place now.
I think it's good to capture the URI when you have it - but you
certainly don't want all the conversions happening elsewhere - it
seems to make sense to support both File and URI as inputs to the API
(though URI need only be in private/protected methods), but only
store it as a URI when it is stored (and do the conversion at the
point of storage to avoid proliferating that code).
On top of this, we have protected (and private) constructors in
this class just to allow support the DRY principle on two fields
(projectId and pomUri). This means that, taken with the file
construction used to support the (now deprecated) string-based
constructors for pomLocation, we have two layers of indirection
feeding a private constructor. This seems geared to avoid
duplication of calls to set these two fields in the large number of
new constructors, but the way I see it, it's a massively over-
normalized approach, and it makes this class much harder to read
with little or no corresponding benefit. IMO, until we have more
than two fields to set, private constructors on these sorts of
exceptions will only confuse their readability.
I don't think readability of the exception class is particularly
concerning - I would be looking at the usages of it and the
javadoc :) But yeah, private constructors in an exception seem
unnecessary, and frankly I think we could do away with the projectId
and come up with a better way to represent a project between the URI
and the ID. The ID gets passed around far more than necessary in the
builder itself.
I guess maybe what we really need here is a better discussion of
exactly how we support the user through errors. I've been thinking
about this stuff quite a bit lately, and I realize it's not enough
to have the information for the topmost and bottom-most exceptions
in the chain. Instead, each place where an exception is wrapped,
it's wrapped because we're traversing a layer in the architecture
(or, it should be this way, in any event). This means that there is
a new layer of context information associated with the site where
the new wrapper is constructed, and this additional context can be
critical in making the error easier to understand. Things like
certain pertinent parameters, project instances that were in play
when the error occurred, extra plugin information pertinent to the
error, and so forth all need to be captured.
I agree.
Here are the guidelines I think through off the top of my head when
doing such things:
- don't be afraid to throw a runtimeexception if the caller shouldn't
be expected to be able to deal with it and it is unrecoverable
- don't be afraid to rethrow the same, unwrapped exception if the
caller would know what it means. For example, if you get passed a
file, and it's not found, throw the FileNotFoundException, don't wrap
it.
- use meaningful parameters in exceptions if they'll help diagnosis
independently of the message
- if you're coding against an api and you catch an exception that you
can't show to the user in a meaningful way right now, then rethrowing
it won't help - something has already gone wrong at the level deeper.
- if you throw a wrapped exception, add information that helps to the
message (or extra parameters) if appropriate. If the existing message
is sufficient, reuse it (or maybe just throw the original exception
as above). But if you don't know that the existing message will be
sufficient, see rule above.
- make sure whatever you throw, the calling code will know what to do
with it as meaningfully as you can in the current code.
- look in nested exceptions and traces for programming errors, not
recoverable problems (eg, resource not found on the repository).
Describe why it happened and you'll know where it happened and so
will the user - and so there should be no need to propagate the
nested exception.
- and all the usual things about being specific (don't toss and catch
Exception and Throwable, etc).
When it comes to actually traversing the tree of possible
exceptions and constructing a coherent error message for the user,
we also need to have some reasonable assurance that we have 90% of
the possible exception chains accounted for. If new functionality
results in new exception chains, we should have to *think* about
the reporting implications of adding these new chains. To me, this
means forcing the developer to deliberately enable the new chain by
adding a constructor for the new sub-exception in the wrapper
exception. It's not perfect, obviously, but it should help us
remember that we need to add reporting code for this new variant of
the wrapper.
The error diagnoser you added was a cool way to figure some of this
out, but it was kind of covering over the mistakes made elsewhere
too :) Ideally, I don't think having to traverse the exception chain
should be necessary based on the guidelines above because you should
always know enough about what went wrong at each level to express it
to the caller explicitly. If you think about a project building
exception - someone using maven-project directly, maven-embedder, and
the CLI should all be able to get the same, meaningful explanation of
a particular failure, right?
WDYT?
Cheers,
Brett
--
Brett Porter - [EMAIL PROTECTED]
Blog: http://www.devzuz.org/blogs/bporter/
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
