SonarQube is happy except for 3 code smells but 2 of them can be marked as “won’t fix” and the third is only a redundant check which can be either ignored or fixed in a patch.
+1 for the release ! Thanks to all the contributors + @luc for the release .
+1 for the release.
I’d also like to highlight a potential improvement in the conversion of osculating elements into mean elements for analytical theories: Brouwer-Lyddane, Eckstein-Hechler, DSST, SGP4/SDP4 (i.e. forTLE).
In fact I consider them as false positive and I have flagged them as such in SonarQube. “NaN” is the accepted proper name for such invalid data, despite is has a lowercase “a”. It is spelled this way in standard Java Doubleclass.
I am not sure about the third case, which is about something that is loaded from either the jar or the development source tree. As it is loaded at runtime, I feel better to still check we can open this stream. I am therefore wondering if we should declare it as “false positive”, as “won’t fix” or consider my concerns are moot and fix it later on.
+1 for the release.
Impressive work indeed!
Thank you @luc for taking care of it!!!
I’ve noticed two classes that have a missing @DefaultDataContext but this is not a no-go for me.
(These are RtcmMessageType and GlobalPressureTemperature2. Search for context in the latest CI log and you’ll find them)
Looking a bit closer, it appears that it is not possible to avoid using the default data context when using those classes. In particular, RtcmMessageType eventually gets UT1 from the default data context, so it would seem that users of that class can’t update their EOP without terminating the JVM.
For the GPT models, using another data context would involve adding a constructor parameter, like in GlobalPressureTemperature or GlobalPressureTemperature3, which could be easily done in a patch release.
RtcmMessageType is an enum and the offending method implements the interface MessageType. It seems the fix would be to add a TimeScales as a method parameter. That would be easier to do in a major release when backwards compatibility is not a concern.
So I’m -0 for the release. I think now is the best time to fix RtcmMessageType, but I don’t want to hold up the release if no one else thinks that is important.
Other than that issue, it is a great set of new features and my initial testing with it has been positive.
I also think that in that context of RTCM which is based on a constant data stream and could potentially be used on an always up service, letting the users update their EOP is mandatory.
I therefore cancel this vote, will fix this issue and prepare a new release candidate.
.
