From: Max Nikulin <manikulin@gmail.com>
To: orgmode <emacs-orgmode@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>
Subject: Re: bug#54764: encode-time: make DST and TIMEZONE fields of the list argument optional ones
Date: Sun, 10 Apr 2022 10:57:21 +0700 [thread overview]
Message-ID: <6b323915-14f9-b049-db01-ceb358ee693f@gmail.com> (raw)
In-Reply-To: <149de00f-115b-5367-414f-c7700ef8966b@cs.ucla.edu>
I am sorry, I have realized that debbugs.gnu.org does not send copies to
the addresses specified in the X-Debbugs-CC header of the original
report. Now I am sending a copy of my earlier message to emacs-orgmode.
Please, add 54764@debbugs.gnu.org to replies to this message.
Paul,
I am sorry that I forced you to make more changes in the Org code. While
I was filing this bug, my intention was to add something like "if
(!NILP(...)) { .... }" around several lines in src/timefns.c.
Now we should decide whether we leave this bug for possible changes in
the `encode-time' implementation and moving the discussion related to
further changes in Org to the emacs-orgmode mail list or we change the
title of this bug and maybe create another one for `encode-time'.
On 09/04/2022 14:52, Paul Eggert wrote:
> On 4/7/22 05:37, Max Nikulin wrote:
>
>> (encode-time '(0 30 20 07 04 2022 nil nil nil)) ; wrong!
>
> Yes, and I see a couple of places (org-parse-time-string,
> org-read-date-analyze) where Org mode returns the wrong decoded
> timestamps, ending in (nil nil nil)
I see you even change `org-store-link' that has the same problem.
> Obsolescent, not obsolete. The old form still works and it's not going
> away any time soon. If the efficiency concerns you mention are
> significant, we should keep the old form indefinitely.
I am against preserving the old form because it ignores the DST field.
It is confusing and error prone to be a part of a well designed interface.
Actually I have a draft of `org-encode-time' macro that transforms 6
elements list to 9 elements one at load or compile time, so it should
not hurt runtime performance. I have not tried to replace all calls of
the `encode-time' function yet however. But I still prefer to drop 6/9
elements branch even if it may happen a decade later.
>> From my point of view it is better to change implementation of
>> `encode-time' so that it may accept 6-component list SECOND...YEAR. It
>> should not add noticeable performance penalty but makes the function
>> more convenient in use.
>
> Unfortunately it makes the function more convenient to use incorrectly.
> This was part of the motivation for the API change. The obsolescent
> calling convention has no way to deal with ambiguous timestamps like
> 2022-11-06 01:30 when TZ="America/Los_Angeles". Org mode surely has bugs
> in this area, although I don't have time to scout them out.
I do not see your point here. Old calling convention did not allow to
specify DST flag at all and it was a problem. With the list argument
even if last 3 fields are optional, it will not prevent adding DST value
when it is known from some source. I think, requiring 3 extra fields
when DST value is unknown is too hing price just to make a developer
aware that it may be ambiguous (moreover it will not work without a
clear statement in the docstring).
Org timestamps does not allow to specify timezone abbreviation such as
PDT/PST to distinguish DST. If it were added then users would have false
impression of full time zones support in Org. It would require huge
amount of work. So guessed DST is the best that often can be offered.
>> Daylight saving time field matters only as a list component and
>> ignored as a separate argument (by the way, it should be stressed in
>> the docstring).
>
> Do you have a wording suggestion? (The doc string already covers the
> topic concisely; however, conciseness is not always a virtue. :-)
My point is that subtle breaking changes must be prominent and hard to
ignore. I do not have yet ready phases and you highlighted another issue
missed in the docstring.
>> So my proposal is to not force Org mode to use new calling convention
>> for `encode-time' till DST and ZONE list components will became
>> optional ones in a released Emacs version.
>
> This would delay things for ten years or so, no? We'd have to wait until
> Org mode supported only Emacs 29 and later.
I do not think it is a problem.
> Instead, I suggest that we stick with what we have when that's cleaner.
> That is, Org mode can use the obsolescent encode-time API when it's
> cleaner to do that.
I considered such approach to defense against "aggressive" modernizing
of Emacs code. Then I decided it is better to allow to deprecate one of
the styles of calling `encode-time'. I tried to express it in the
original report and above.
> I haven't installed the patch, or tested it other than via 'make check'.
Org has its own repository and changes should be committed there at first.
Org has unit tests, see
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/testing/README
you changes should be accompanied by some adjustments in test expectations.
I believe that at least in some cases Org test suite should be run for
the bundled version of Org after Emacs builds. There are may be
copyright issues with including the tests into the Emacs source tree.
Maybe some changes in tests were accepted without paperwork.
> PS. Org mode usually uses encode-time for calendrical calculations. This
> is dicey, as some days don't exist (for example, December 30, 2011 does
> not exist if TZ="Pacific/Apia", because Samoa moved across the
> International Date Line that day). And it's also dicey when Org mode
> uses 00:00:00 (midnight at the start of the day) as a timestamp
> representing the entire day, as it's all too common for midnight to not
> exist (or to be duplicated) due to a DST transition. Generally speaking,
> when Org mode is doing calendrical calculations it should use
> calendrical functions rather than encode-time+decode-time, which are
> best used for time calculations not calendar calculations. (I realize
> that fixing this in Org would be nontrivial; perhaps I should file this
> "PS" as an Org bug report for whoever has time to fix it....)
I agree with you that dates should not be represented as timestamps and
date modifications (day, week, month, year increments and decrements)
should be performed by dedicated functions. I even had 2011-12-30
example in my notes. However I expect that underlying normalization of
date-time fields may mitigate such issues to some extent.
> diff --git a/lisp/org/org-compat.el b/lisp/org/org-compat.el
> index 819ce74d93..247373d6b9 100644
> --- a/lisp/org/org-compat.el
> +++ b/lisp/org/org-compat.el
> @@ -115,6 +115,27 @@ org-table1-hline-regexp
> (defun org-time-convert-to-list (time)
> (seconds-to-time (float-time time))))
>
> +;; Like Emacs 27+ `encode-time' with one argument.
> +(if (ignore-errors (encode-time (decode-time)))
> + (defsubst org-encode-time-1 (time)
> + (encode-time time))
> + (defun org-encode-time-1 (time)
> + (let ((dst-zone (nthcdr 7 time)))
> + (unless (consp (cdr dst-zone))
> + (signal wrong-type-argument (list time)))
> + (let ((etime (apply #'encode-time time))
> + (dst (car dst-zone))
> + (zone (cadr dst-zone)))
> + (when (and (symbolp dst) (not (integerp zone)) (not (consp zone)))
> + (let* ((detime (decode-time etime))
> + (dedst (nth 7 detime)))
> + (when (and (not (eq dedst dst)) (symbolp dedst))
> + ;; Assume one-hour DST and adjust the timestamp.
> + (setq etime (time-add etime (seconds-to-time
> + (- (if dedst 3600 0)
> + (if dst 3600 0))))))))
> + etime))))
> +
> ;; `newline-and-indent' did not take a numeric argument before 27.1.
> (if (version< emacs-version "27")
> (defsubst org-newline-and-indent (&optional _arg)
> diff --git a/lisp/org/org-macro.el b/lisp/org/org-macro.el
> index 0921f3aa27..b8e4346002 100644
I do not have complicated agenda setup to profile performance after such
change. I will post my simple macro when we decide to continue
discussion on debbugs or in a dedicated thread of the emacs-orgmode list.
In my opinion, the code obtaining DST value requires unit tests.
I like you idea to reuse `org-time-string-to-seconds' in more places. My
original plan was to use `org-time-string-to-time' there.
next prev parent reply other threads:[~2022-04-10 3:58 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 12:37 bug#54764: encode-time: make DST and TIMEZONE fields of the list argument optional ones Max Nikulin
2022-04-09 7:52 ` Paul Eggert
2022-04-10 3:57 ` Max Nikulin [this message]
2022-04-13 14:40 ` Max Nikulin
2022-04-13 18:35 ` Paul Eggert
2022-04-14 13:19 ` Max Nikulin
2022-04-14 22:46 ` Paul Eggert
2022-04-15 2:14 ` Tim Cross
2022-04-15 17:23 ` Max Nikulin
2022-04-16 19:23 ` Paul Eggert
2022-04-21 16:59 ` Max Nikulin
2022-04-19 2:02 ` Paul Eggert
2022-04-19 5:50 ` Eli Zaretskii
2022-04-19 22:22 ` Paul Eggert
2022-04-20 7:23 ` Eli Zaretskii
2022-04-20 18:19 ` Paul Eggert
2022-04-20 18:41 ` Eli Zaretskii
2022-04-20 19:01 ` Paul Eggert
2022-04-20 19:14 ` Eli Zaretskii
2022-04-20 19:23 ` Paul Eggert
2022-04-20 19:30 ` Eli Zaretskii
2022-04-21 0:11 ` Paul Eggert
2022-04-21 6:44 ` Eli Zaretskii
2022-04-21 23:56 ` Paul Eggert
2022-04-22 5:01 ` Eli Zaretskii
2022-04-23 14:35 ` Bernhard Voelker
2022-04-20 15:07 ` Max Nikulin
2022-04-20 18:29 ` Paul Eggert
2022-04-25 15:30 ` Max Nikulin
2022-04-25 15:37 ` Paul Eggert
2022-04-25 19:49 ` Paul Eggert
2022-04-30 11:22 ` Max Nikulin
2022-05-01 2:32 ` Paul Eggert
2022-05-01 17:15 ` Max Nikulin
2022-04-13 15:12 ` Max Nikulin
2022-04-16 16:26 ` Max Nikulin
2022-04-17 1:58 ` Paul Eggert
2022-04-20 16:56 ` Max Nikulin
2022-04-20 19:17 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6b323915-14f9-b049-db01-ceb358ee693f@gmail.com \
--to=manikulin@gmail.com \
--cc=eggert@cs.ucla.edu \
--cc=emacs-orgmode@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).