[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Orekit Developers] Ephemeris Writer Interface





On 10/27/2016 04:26 AM, Luc Maisonobe wrote:
Le 26/10/2016 à 15:32, MAISONOBE Luc a écrit :
Evan Ward <evan.ward@nrl.navy.mil> a écrit :

I pushed up a commit that added the step size to the init(...) method.
I think this helps reduce the duplicated information that could become
mismatched.
This is fine with me. With this change and Pascal's change about
atmosphere, I guess next version will be 9.0.

There is a problem with this change.

The TLEPropagatorTest.testBodyCenterInPointingDirection() does not pass
anymore. The reason is that is uses a DistanceChecker instance, which
implements OrekitFixedStepHandler but using the previous signature for
init(), without the step size. As the new init has a default
implementation that does nothing, the test compiles ans the default
implementation is used, not the DistanceChecker.init() specialization.

In order to fix this, I added back init(SpacecraftState s0,
AbsoluteDate t), deprecate it, and had the default implementation
of init(SpacecraftState s0, AbsoluteDate t, double step) call this
former method. Then, all existing code still run.


Luc, thanks for finding and fixing that issue. My opinion is that users should be using the @Override annotation to ensure that a method actually overrides the definition in the interface. I can add these to our test code.


One glitch is that despite I have annotated the older init method
as deprecated, when some class overrides it there are no warnings, so
user are not aware this method should be replaced by the new signature.
I don't know how to handle this (except simply putting a comment in the
release notes). Does anyone has an idea?

I see warnings both in my IDE (IDEA) and in the maven output:

[WARNING] /home/ward/code/orekit/src/test/java/org/orekit/propagation/events/DetectorTest.java:[110,21] init(org.orekit.propagation.SpacecraftState,org.orekit.time.AbsoluteDate) in org.orekit.propagation.sampling.OrekitFixedStepHandler has been deprecated


There is another small glitch somewhere else, due to a design error I
made months ago, in some test cases. The ElevationDetectorTest.Checking
and DetectorTest.OutOfOrderChecker internal classes implement *both*
EventHandler and OrekitFixedStepHandler, so they inherits *two* init
methods, now with different signatures. So I had to create two
implementations now, and the compiler does not really help noticing
the problem.

That is a tricky problem to find, though I think your current solution does fix it. I think the deprecation warnings will help users in finding it.


I have committed the fixes (and removed a bunch of other init
implementations that did the same thing as the default). Could you
have a look at it?

Looks good. I'll add some docs.

Best Regards,
Evan