On Mon, 21 Dec 2015 12:41:01 -0700, Phil Steitz wrote:
On 12/21/15 11:16 AM, Gilles wrote:
On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
On 12/21/15 8:21 AM, Gilles wrote:
On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
On 12/20/15 8:41 PM, Gilles wrote:
On Sat, 19 Dec 2015 11:35:26 -0700, Phil Steitz wrote:
On 12/19/15 9:02 AM, Gilles wrote:
Hi.
While experimenting on
https://issues.apache.org/jira/browse/MATH-1300
I created a new
JDKRandomGeneratorTest
that inherits from
RandomGeneratorAbstractTest
similarly to the classes for testing all the other RNG
implemented
in CM.
The following tests (implemented in the base class) failed:
1.
testNextIntNeg(org.apache.commons.math4.random.JDKRandomGeneratorTest)
Time elapsed: 0.001 sec <<< ERROR!
java.lang.Exception: Unexpected exception,
expected<org.apache.commons.math4.exception.MathIllegalArgumentException>
but was<java.lang.IllegalArgumentException>
2.
testNextIntIAE2(org.apache.commons.math4.random.JDKRandomGeneratorTest)
Time elapsed: 0.015 sec <<< ERROR!
java.lang.IllegalArgumentException: bound must be positive
This is caused by try/catch clauses that expect a
"MathIllegalArgumentException"
but "JDKRandomGenerator" extends "java.util.Random" that for
those
methods throws
"IllegalArgumentException".
What to do?
I would change the test to expect IllegalArgumentException.
Most
[math] generators actually throw NotStrictlyPositiveException
here,
which extends MIAE, which extends IAE, so this should work.
It turns out that, in the master branch, the hierarchy is
RuntimeException
|
MathRuntimeException
|
MathIllegalArgumentException
as per
https://issues.apache.org/jira/browse/MATH-853
[And the Javadoc and "throws" clauses are not yet consistent
with
this
in all the code base (e.g. the "RandomGenerator" interface).]
So, in 4.0, "JDKRandomGenerator" should probably not inherit
from
"java.util.Random" but delegate to it, trap standard exceptions
raised,
and rethrow CM ones.
I guess that is the only way out if we are going to stick with
the
MATH-853 setup. This example illustrates a drawback identified
in
the thread mentioned in the ticket. I would personally be happy
reverting master back to the 3.x setup.
We can't throw (!) the many discussions that led to MATH-853 by
just
reverting on the basis of the current issue.
Perhaps that the multiple hierarchies are better, maybe not.
But we have to figure out arguments that are more convincing than
nostalgia.
For instance, IMO, we have contradicting wishes:
* Have CM exception inherits from the JDK ones with the same
semantics.
* Have all CM exceptions provide the same additional functionality
(the
"ExceptionContext").
In 3.x, MIAE extends IAE and we have all the context stuff working.
I guess you are concerned about the "duplicated code" in the
multiple roots extending the standard exceptions. Here is the full
extent of it:
/**
* @param pattern Message pattern explaining the cause of the
error.
* @param args Arguments.
*/
public MathIllegalArgumentException(Localizable pattern,
Object ... args) {
context = new ExceptionContext(this);
context.addMessage(pattern, args);
}
/** {@inheritDoc} */
public ExceptionContext getContext() {
return context;
}
/** {@inheritDoc} */
@Override
public String getMessage() {
return context.getMessage();
}
/** {@inheritDoc} */
@Override
public String getLocalizedMessage() {
return context.getLocalizedMessage();
}
I see that as NOT_A_PROBLEM. In 3.x, we have 6 classes that need
this little bit of similar code. That is after 10 years of
development. I doubt we will have more than a few more needed at
the top level.
It's not a problem of how many lines of code must be duplicated.
The problem is the duplication.
Duplication is not a best practice AFAIK.
I agree that duplication is to be avoided; but I think it does make
a difference how much / how complex the code is. The duplicated
code in this case is trivial.
Code duplication is a sure sign that the design must be questioned,
and that a better alternative probably exists.
The first looks nice from a user POV (as in "I know this
already").
The second is an internal requirement to avoid code duplication.
IMO, it always comes back to those opposite views we have
concerning the
place where human-readable messages should be generated: my view
is that
if a low-level library message bubbles up to the console, then
it's not
our fault, but the application programmer's.
Meaningful exception messages are a best practice in Java. They
are
a very useful aid in debugging and production support.
Please assume that for a moment; then CM could have its algorithms
throw
some internal subclass of "MathRuntimeException" (whose type is
known to
the caller) and the caller can decide whether he must trap it in
order
to rethrow a standard type, or his own type (tailored to the
context which
he knows but CM doesn't).
In that scenario, it is much easier for the caller when the
low-level
library's exceptions hierarchy is unique (especially when he uses
several
libraries).
And it's also easier for us because we can avoid code duplication.
Easiest is if the library follows the Java best practice, which is
to favor standard exceptions.
We do not favour standard exceptions because we _have_ to define our
own.
And we have done this for various "good" reasons in line with other
best practices, subject to internal requirements.
We can have it both ways if we define our own exceptions to extend
the standard exceptions where that makes sense, which is the best
practice. Having MIAE extend IAE is good design, IMO. What we are
throwing on bad arguments is IAE. Callers can catch that. If they
want to dig in deeper and differentiate their handlers based on
which of our IAE subclasses is thrown, they can do that. That is
how exception hierarchies are supposed to work. The "favor standard
exceptions" principle is there so people don't go and define "new"
exceptions that are just renamed versions of the standard
exceptions. That is precisely what MIAE does in V4. The base MIAE
*is* IAE, just renamed, disconnecting all of the exceptions derived
from it from their natural root, which is IAE.
I agree that reuse is better.
As Luc said, we would have reused standard exceptions if they were
like interfaces.
There is a technical (creating duplicate code), because of how
inheritance works in Java, so I don't think that you can consider it
obvious that inheritance must be the way to go.
Then these can be caught without
rummaging through library-specific javadoc to figure out what
unchecked exception means what. This is especially true for
unchecked exceptions.
A standard exception does not mean anything specific.
It does. That's the point. IAE means you have provided bad
arguments, ISE means you have encountered bad state.
ArithmeticException means an exceptional condition has been
encountered performing an arithmetic operation. We should, where
natural, derive from these.
They are non-specific in the sense which you usually use when you
say that CM must provide a _detailed_ report of the failure.
If the details are provided by the message, the type of the exception
is not that important (compared to the cleanliness of the code for
creating of the message).
If the failure's cause is conveyed by exception type, then it is more
important to reuse existing types.
So why bother with the "implementation detail"?
try {
/** any code that deep down may or may not call CM */
} catch (RuntimeException e) {
// Deal with it or rethrow.
}
will work, always. User does not need to know anything to print
and read the meaningful message, only JDK's "RuntimeException" here.
The problem is _not_ there.
Not sure I follow what you mean.
I tried to make it clearer in the previous paragraph.
You never convinced me that you were aware of this type of
scenario and
why, somehow, it was trumped by a nicely formatted message on the
console.
The more so that a single hierarchy does not prevent the latter:
at the point where the message is printed, the exception type is
useless.
I think we can have it both ways. The 3.x setup allows us to
extend
standard exceptions *and* provide good exception messages.
The question is: Why extending standard exceptions?
We came to another answer in MATH-853.
So the problem cannot be solved as if it did not exist.
I guess I am seeing more consequences now of completely proprietary
exceptions. It bothered me then. It bothers me more now as I
think about the consequences for client code.
As indicated by Ole in the other, I think that it makes sense for
an application/library to shield its callers from the possibly many
different exception types used by the libraries it uses.
It implies that even if CM were to throw standard exception instances
where appropriate according to the standard exception semantics, an
application's developer keen to provide a uniform interface to its
users
will wrap them into his own application-specific exceptions (that
could,
or not, extend the standard exceptions).
The CM developers seem strongly biased by the fact that they are also
those users of CM satisfied with letting the CM exception types bubble
up to the top.
IOW, the low level is coded according to what we like to see at the
top level. But how did we conclude that all developers would be fine
with a simple text message?
For example, if they want to restart a process depending on the cause
of the failure, it is much easier to examine the exception type rather
than parse the text message (the more so if it is localized!).
I must have mentioned this more than a few times along the years. :-}
To summarize, if the top-level message matters most, then I'd be
-1 to revert
(because that's an orthogonal issue); if having standard types
matters most,
I'd wait for use-cases that show how these types are used in the
caller's
code before trying to figure out how to satisfy them (while taking
our
internal requirements in to account too).
Well, we have a nice example in front of us in our own test case
here. But the bigger deal is the pain for every user who when
migrating from 3.x to 4 has to stop catching standard IAE if they
did this and replace it with MIAE. Note that the compiler will be
no help there - failure to "comply" will cause the client app to
"go
boom."
All this has been said again and again.
I'd like that we do not take our own assumptions for granted
(be they the scenario which I described, or that compatibility
is the ultimate goal, even if the design is crap) but instead
that we could talk about what a live project CM can be, looking
to what can be done in possibly novel ways.
A new major version is an opportunity to legally break
compatibility in order to try new things.
Instead we have full-time discussions to avoid changing anything.
Not a fair statement. I want to make sure that the changes we make
are positive.
I don't doubt it.
But I think that we have the right to be wrong while trying to make
a better library.
What is not fair is your statement that "client app [will go] boom":
It is not true, because they can continue using 3.x if they are not
willing to deal with a new library.
The only people annoyed are those who decide that they do not want
to use legacy code, but also do not want to deal with the consequences
of their _own_ decision.
Of course, there is the problem of the resources needed to maintain
legacy versions of CM.
But it is not fair that this should prevent compatibility-breaking
changes forever. It would be fair that those users who need legacy
code dedicate some of their time to its maintenance.
As Ole proposed, it might worth be splitting CM into modules that
could be release separately. Those modules on which few others
depend could then be released more often.
Gilles
Phil
Users are never forced to follow development.
Gilles
Phil
In conclusion, let's not jump to conclusions. ;-)
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
