Re: [DRAFT][PATCH v2] org-encode-time compatibility and convenience helper

[Max Nikulin]

On 23/04/2022 15:25, Ihor Radchenko wrote:
> Max Nikulin writes:
>
> > My patch requires more changes since the macro is just defined but not
> > actually used. It does not fix the problem with "no DST" flag returned
> > by some function in Org. I can prepare next patches, but I think it
> > should be decided at first which approach should be accepted by Org Mode:
> > - org-encode-time accepting both list or separate arguments
> > - mix of `encode-time' with multiple arguments and org-encode-time-1 for
> > lists.
>
> This whole timezone staff is complex. I got lost in the emacs devel
> discussion half-way through. From point of view of API, I would prefer a
> single function with docstring explaining the necessary caveats.

To have namely a single function that accepts both a list or multiple 
arguments, it is necessary to introduce a new name to Emacs. 
`encode-time' has a subtle difference in behavior depending on the 
calling style and fixing this issue may break some code. That is why I 
decided to offer a macro.

I have not figured out which kind of high-level API I would like to have 
instead of `encode-time', and I believe, it is acceptable to rely on 
default normalization and ambiguity resolution as the status quo. 
Problems may happen during 2 days of 365 and people usually expect some 
glitches for these days. There are too many caveats to explain them in 
docstring.

> > +      (if (cdr time)
> > +          `(encode-time ,@time)
> > +        `(apply #'encode-time ,(car time))))
>
> Do I miss something or can you instead just do `(encode-time ,@time)
> without if?

I changed the code to use ,@time in both cases. Previous time likely I 
forgot to re-evaluate some definition, got some unexpected error, and 
decided to use `car' for a while.

`if' can not be dropped however. In the case of
    (org-encode-time '(0 0 23 30 04 2022 nil nil nil))
`(encode-time ,@time) will be expanded to
    (encode-time (list 0 0 23 30 04 2022 nil nil nil))
due to (&rest time) argument. Single list argument is unsupported in 
Emacs-26. So `apply' cat not be avoided.

> > +  (should (string-equal
> > +           "2022-03-24 23:30:01"
> > +           (format-time-string
> > +            "%F %T"
> > +            (org-encode-time '(01 30 23 24 03 2022 nil -1 nil)))))
> > ...
>
> These tests will be executed using system value of TZ. I am not sure if
> tests are not going to break, say, in southern hemisphere or at some
> other bizzare values of TZ.

You are right, it is safer to run this test with explicitly chosen TZ 
value. I do not think there is a point in speculation whether the test 
fails in some currently existing time zone.

I am attaching an updated version of the draft. I have added a macro for 
testing of particular time zones.

0001-org-macs.el-Introduce-a-helper-for-encode-time.patch
Description: Text Data