https://bz.apache.org/bugzilla/show_bug.cgi?id=65217

--- Comment #3 from Mariusz_W <mawa...@gmail.com> ---
Felix, thanks for the PR, I wasn't aware it existed. 

In current implemetation there is some additional calculations because function
(timeShift) always use ZonedDateTime  even for input date without time zone).
In such case we have default offset in DataFormatter (in
TimeShift.createFormatter there is parseDefaulting(ChronoField.OFFSET_SECONDS,
ZonedDateTime.now().getOffset().getTotalSeconds())  ) which may cause errors
because now() is used and we get something like 2021-03-27T15:00+02:00 (in CET)
which is not existing date. It should be 2021-03-27T15:00+01:00, it's one day
before change from CET (+01) to CEST (+02). Additionaly there are some
inconsistency - if we use timeShift function as in testNowWithComplexPeriod()
where now() is used we can get error as described in issue however  If we use
time on input e.g. 2021-03-27T15:00:00  (day before DST) and add "P1D" we don't
have error because offset is used in createFormatter and DST is not managed.

>From my point of view (and docs) I deduce that this function (timeShift) should
by simple one point interface to Java time api. Function should recognize input
format with time zone or without time zone and make adequate calculations. When
time zone is present in input format the  ZoneDateTime class should be used. In
case of time without zone LocalDateTime class should be used. Duration class is
used not Period class of course in implementation. This is first approach. 

The second approach is always use ZonedDateTime (even for input string without
zone) but make calculations based on system zone. This is like current
implementation but in TimeShift.createFormatter  this line is not used
.parseDefaulting(ChronoField.OFFSET_SECONDS,
ZonedDateTime.now().getOffset().getTotalSeconds())  but
withZone(ZoneId.systemDefault())  is used. This change prevents problem I
described at beginning. I think personally that this approach is less intuitive
because on input there is no time zone but timeShift is using time zone
calculations internally. When we use LocalDateTime the operation "P1D" on
Duration and Period get the same result on DSP because zone is not used in
calculations.

After this lenghty disquisition:) I think that maybe not only test in  test
class should  be changed but also TimeShift implementation. 
What do you think? Do I miss something?

I will try to work on it and present some pull request.

-- 
You are receiving this mail because:
You are the assignee for the bug.

Reply via email to