mosaic icon indicating copy to clipboard operation
mosaic copied to clipboard

feat(mapping): Use Nanoseconds for vehicle-config time fields

Open schwepmo opened this issue 3 years ago • 2 comments

Type of this PR

  • [ ] Bug fix
  • [x] Enhancement

Description

  • All time values in vehicle and vehicle flow generations are now treated in nanoseconds and are stored as longs instead of doubles
  • Now the TimeFieldAdapter.NanoSeconds is used for all time-values, i.e. startingTime, maxTime
  • this has the consequence that all mapping_config.json needed to be adjusted
  • some cleanup

Issue(s) related to this PR

  • Relates to internal issue 421

Affected parts of the online documentation

  • Yes all occurences of vehicle mappings need to be adjusted accordingly

Definition of Done

  • [x] You have read CONTRIBUTING.md carefully.
  • [x] You have signed the Contributor License Agreement.
  • [x] All commits are signed-off (-s) with your mail address of your Eclipse Account.
  • [x] origin/main has been merged into your Fork.
  • [x] New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
  • [x] There are no new SpotBugs warnings.
  • [x] Coding guidelines have been observed (see CONTRIBUTING.md).
  • [x] All tests pass.

Special notes to reviewer

schwepmo avatar Oct 04 '22 15:10 schwepmo

Phew, I remember that we intentionally decided to not migrate those fields to long due to the required changes in all mapping_config. I also preferred that we could still use numbers instead of strings in the mapping for the starting times, making them much easier to write and read. Maybe we can fix this somehow that we still can configure raw numbers as seconds which are translated by the TimeFieldAdapter to nanoseconds? However, this is still not very clean though and I'm not sure if this is the solution. I can see the intention behind this this change (e.g., consistency), but I'm not sure if we really require it.. convince me :)

kschrab avatar Oct 05 '22 06:10 kschrab

Phew, I remember that we intentionally decided to not migrate those fields to long due to the required changes in all mapping_config. I also preferred that we could still use numbers instead of strings in the mapping for the starting times, making them much easier to write and read. Maybe we can fix this somehow that we still can configure raw numbers as seconds which are translated by the TimeFieldAdapter to nanoseconds? However, this is still not very clean though and I'm not sure if this is the solution. I can see the intention behind this this change (e.g., consistency), but I'm not sure if we really require it.. convince me :)

Yeah, I'm not so sure either about this change. We could default to TimeFiieldAdapter.LegacySeconds but then it wouldn't really be "Legacy" anymore and this would also require the values to be longs in order for the TimeFieldAdapter to work.

In the long run I feel like this change will lead to better consistency and user acceptance, as you won't be suprised that you can't use "1 s" for vehicle mappings. Additionally, we get rid of some confusing type casts.

An alternative approach would be to use two json fields, where one takes precedence over the other, i.e. startingTime -> long using TimeFieldAdapter and startingTimeInS -> double.

As I said, I'm not completly convinced either about this change and we should probably discuss this in person.

schwepmo avatar Oct 05 '22 07:10 schwepmo

To resume discussion, I would propose that we do NOT change the time field to long/nanoseconds but to provide a TimeFieldAdapter which is able to transform string representation to double (seconds). I attached an updated version of the TimeFieldAdapter, which provides additional nested classes for conversion of string to double (seconds): TimeFieldAdapter.zip

You can just use the annotation to a double field:

@JsonAdapter(TimeFieldAdapter.DoubleSeconds.class)
double startingTime = 0;

Or for Double if null should be allowed:

@JsonAdapter(TimeFieldAdapter.DoubleSecondsNullable.class)
Double maxTime;

kschrab avatar Dec 02 '22 10:12 kschrab

To resume discussion, I would propose that we do NOT change the time field to long/nanoseconds but to provide a TimeFieldAdapter which is able to transform string representation to double (seconds). I attached an updated version of the TimeFieldAdapter, which provides additional nested classes for conversion of string to double (seconds): TimeFieldAdapter.zip

You can just use the annotation to a double field:

@JsonAdapter(TimeFieldAdapter.DoubleSeconds.class)
double startingTime = 0;

Or for Double if null should be allowed:

@JsonAdapter(TimeFieldAdapter.DoubleSecondsNullable.class)
Double maxTime;

I took this suggestion and implemented it in PR #277. I suggest closing this PR and deleting respective branches!

schwepmo avatar Dec 05 '22 14:12 schwepmo

Resolved with #277

kschrab avatar Dec 07 '22 08:12 kschrab