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

[Max Nikulin <manikulin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

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.

[-- Attachment #2: 0001-org-macs.el-Introduce-a-helper-for-encode-time.patch --]
[-- Type: text/x-patch, Size: 4647 bytes --]

From 8fdcca3f3b0cf3e890149eb79ea6c7970b7d2cfa Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Fri, 8 Apr 2022 23:10:50 +0700
Subject: [PATCH] org-macs.el: Introduce a helper for `encode-time'

* lisp/org-macs.el (org-encode-time): New compatibility and convenience
helper macro to allow a list for time components or separate arguments
independently of Emacs version.
* testing/lisp/test-org.el (org-test-with-timezone): New macro to ensure
that some code is executed with or without daylight saving time or other
time shifts by setting certain TZ environment value.
* testing/lisp/test-org.el (test-org/org-encode-time): Tests for various
ways to call `org-encode-time'.

Ensure recommended way to call `encode-time' for Emacs-27 and newer with
hope to avoid bugs due to attempts to modernize the code similar to
bug#54731.
---
 lisp/org-macs.el         | 20 ++++++++++++++
 testing/lisp/test-org.el | 56 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index a09115e7c..85dd20028 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1225,6 +1225,26 @@ nil, just return 0."
 	(b (org-2ft b)))
     (and (> a 0) (> b 0) (\= a b))))
 
+(if (version< emacs-version "27.1")
+    (defmacro org-encode-time (&rest time)
+      (if (cdr time)
+          `(encode-time ,@time)
+        `(apply #'encode-time ,@time)))
+  (defmacro org-encode-time (&rest time)
+    (pcase (length time)
+      (1 `(encode-time ,(car time)))
+      (6 `(encode-time (list ,@time nil -1 nil)))
+      (9 `(encode-time (list ,@time)))
+      (_ (error "`org-encode-time' may be called with 1, 6, or 9 arguments but %d given"
+                (length time))))))
+(put 'org-encode-time 'function-documentation
+     "Compatibility and convenience helper for `encode-time'.
+May be called with 9 components list (SECONDS ... YEAR IGNORED DST ZONE)
+as the recommended way since Emacs-27 or with 6 or 9 separate arguments
+similar to the only possible variant for Emacs-26 and earlier.
+Warning: use -1 for DST that means guess actual value, nil means no
+daylight saving time and may be wrong at particular time.")
+
 (defun org-parse-time-string (s &optional nodefault)
   "Parse Org time string S.
 
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 6aecc3af8..aa3f9a3f6 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -24,6 +24,20 @@
 
 (eval-and-compile (require 'cl-lib))
 
+\f
+;;; Helpers
+
+(defmacro org-test-with-timezone (tz &rest body)
+  "Evaluate BODY with TZ environment temporary set to the passed value."
+  (declare (indent 1))
+  (org-with-gensyms (tz-saved)
+    `(let ((,tz-saved (getenv "TZ")))
+       (unwind-protect
+           (progn
+             (setenv "TZ" ,tz)
+             ,@body)
+             (setenv "TZ" ,tz-saved)))))
+
 \f
 ;;; Comments
 
@@ -179,6 +193,48 @@
 \f
 ;;; Date and time analysis
 
+(ert-deftest test-org/org-encode-time ()
+  "Test various ways to call `org-encode-time'"
+  (org-test-with-timezone "UTC"
+    ;; list as the sole argument
+    (should (string-equal
+             "2022-03-24 23:30:01"
+             (format-time-string
+              "%F %T"
+              (org-encode-time '(1 30 23 24 3 2022 nil -1 nil)))))
+    ;; SECOND...YEAR
+    (should (string-equal
+             "2022-03-24 23:30:02"
+             (format-time-string
+              "%F %T"
+              (org-encode-time 2 30 23 24 3 2022))))
+    ;; SECOND...YEAR IGNORED DST ZONE
+    (should (string-equal
+             "2022-03-24 23:30:03"
+             (format-time-string
+              "%F %T"
+              (org-encode-time 3 30 23 24 3 2022 nil -1 nil))))
+    ;; function call
+    (should (string-equal
+             "2022-03-24 23:30:04"
+             (format-time-string
+              "%F %T"
+              (org-encode-time (apply #'list 4 30 23 '(24 3 2022 nil -1 nil))))))
+    ;; wrong number of arguments
+    (if (not (version< emacs-version "27.1"))
+        (should-error (string-equal
+                       "2022-03-24 23:30:05"
+                       (format-time-string
+                        "%F %T"
+                        (org-encode-time 5 30 23 24 3 2022 nil))))))
+  ;; daylight saving time
+  (org-test-with-timezone "Europe/Madrid"
+    (should (string-equal
+             "2022-03-31 23:30:06"
+             (format-time-string
+              "%F %T"
+              (org-encode-time 6 30 23 31 3 2022))))))
+
 (ert-deftest test-org/org-read-date ()
   "Test `org-read-date' specifications."
   ;; Parse ISO date with abbreviated year and month.
-- 
2.25.1