From: Morgan Smith <morgan.j.smith@outlook.com>
To: Ihor Radchenko <yantar92@posteo.net>
Cc: emacs-orgmode@gnu.org, Sanel Zukan <sanelz@gmail.com>
Subject: Parse malformed clocklines (was: Re: [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx)
Date: Mon, 24 Jun 2024 08:09:44 -0400 [thread overview]
Message-ID: <CH3PR84MB34243D1F564538B01F270DE5C5D42@CH3PR84MB3424.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <87h6do7zj6.fsf@localhost> (Ihor Radchenko's message of "Thu, 20 Jun 2024 09:07:41 +0000")
[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]
Ihor Radchenko <yantar92@posteo.net> writes:
> Morgan Smith <morgan.j.smith@outlook.com> writes:
>
>>> That's expected.
>>> We have the following _syntax_ description for clock lines:
>>>
>>> https://orgmode.org/worg/org-syntax.html#Clocks...>> clock: INACTIVE-TIMESTAMP-RANGE DURATION
>> ...
>> My specific issue is that the ":*-end" stuff can be set when the
>> "*-start" stuff is not.
>> ...
>> CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] => 0:46
>> calculated time: 58320.0
>> ...
>> What this shows is that an invalid end will cause the entry to be
>> ignored, but an invalid start will not.
>
> Yup. Everything makes sense, but only within syntax spec. Actual code
> that handles such clock lines is another story.
>
> Warning is important because a number of people use clock functionality
> for billing - miscalculating clock sums can literally affect people's
> income :)
>
> I think that the best course of action when a problematic timestamp
> without opening/closing time is encountered is:
>
> 1. Warn user
> 2. Still calculate the duration, assuming 0s in time (simply because
> previous versions of Org mode did it)
>
> (2) is kind of debatable, but I can imagine some users might make use of
>
> such feature by putting clocks by hand:
> CLOCK: [2023-11-15 Wed]--[2023-11-16 Thu] => 24:00
>
> These users may then simply suppress the warning.
I'm very tempted to just make it hard rule that clocklines need to be
fully specified. If you take a look at the attached patch, it shows one
way we could do this.
First, I modify the timestamp parser so the ":*-end" variables don't
always inherit the ":*-start" variables. They can be nil independent of
the start.
Then in the clockline parser I ensure that every single field is set.
If it isn't, then I set the timestamp to nil so it won't be used.
This preserves the flexibility you want in timestamp parsing while also
catching many malformed clocklines.
Let me know what you think of this approach.
Also a couple questions about this approach:
1. Will changing the timestamp parser have far-reaching implications?
Is this something I should avoid?
2. Is there a way to give user errors in the parser code? I'm using
`org-element--cache-warn' in my patch but I'm not sure that's the right
thing to do. (if it is the right thing then we need to define it
earlier in the file)
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-about-invalid-clock-lines.patch --]
[-- Type: text/x-patch, Size: 3056 bytes --]
From 82231fb4b11b780488c66b4e5f0ee6fcf6643f1d Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Wed, 19 Jun 2024 13:41:50 -0400
Subject: [PATCH] Warn about invalid clock lines
---
lisp/org-element.el | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/lisp/org-element.el b/lisp/org-element.el
index d6ad9824a..113cd6059 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -2312,6 +2312,25 @@ Return a new syntax node of `clock' type containing `:status',
(unless (bolp) (skip-chars-forward " \t"))
(count-lines before-blank (point))))
(end (point)))
+ (when (and value ;; Clock lines don't need timestamps
+ (or
+ (not
+ (and (org-element-property :year-start value)
+ (org-element-property :month-start value)
+ (org-element-property :day-start value)
+ (org-element-property :hour-start value)
+ (org-element-property :minute-start value)))
+ (and (eq status 'closed)
+ (not (and (org-element-property :year-end value)
+ (org-element-property :month-end value)
+ (org-element-property :day-end value)
+ (org-element-property :hour-end value)
+ (org-element-property :minute-end value))))))
+ (setq value nil)
+ (org-element--cache-warn "Invalid clock element at %s:%d: \"%s\""
+ (buffer-name)
+ (line-number-at-pos begin t)
+ (buffer-substring-no-properties begin end)))
(org-element-create
'clock
(list :status status
@@ -4393,15 +4412,14 @@ Assume point is at the beginning of the timestamp."
hour-start (nth 2 date)
minute-start (nth 1 date))))
;; Compute date-end. It can be provided directly in timestamp,
- ;; or extracted from time range. Otherwise, it defaults to the
- ;; same values as date-start.
+ ;; or extracted from time range.
(unless diaryp
(let ((date (and date-end (org-parse-time-string date-end t))))
- (setq year-end (or (nth 5 date) year-start)
- month-end (or (nth 4 date) month-start)
- day-end (or (nth 3 date) day-start)
- hour-end (or (nth 2 date) (car time-range) hour-start)
- minute-end (or (nth 1 date) (cdr time-range) minute-start))))
+ (setq year-end (or (nth 5 date) (and time-range year-start))
+ month-end (or (nth 4 date) (and time-range month-start))
+ day-end (or (nth 3 date) (and time-range day-start))
+ hour-end (or (nth 2 date) (car time-range))
+ minute-end (or (nth 1 date) (cdr time-range)))))
;; Diary timestamp with time.
(when (and diaryp
(string-match "\\([012]?[0-9]\\):\\([0-5][0-9]\\)\\(-\\([012]?[0-9]\\):\\([0-5][0-9]\\)\\)?" date-start))
--
2.45.1
next prev parent reply other threads:[~2024-06-24 12:15 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 17:20 [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx Morgan Smith
2024-04-13 14:49 ` Ihor Radchenko
2024-04-13 16:08 ` Morgan Smith
2024-04-13 16:48 ` Ihor Radchenko
2024-04-13 17:46 ` Morgan Smith
2024-06-18 7:39 ` Ihor Radchenko
2024-06-19 14:57 ` Morgan Smith
2024-06-19 15:46 ` Ihor Radchenko
2024-06-19 18:24 ` Morgan Smith
2024-06-20 9:07 ` Ihor Radchenko
2024-06-24 12:09 ` Morgan Smith [this message]
2024-06-26 9:02 ` Parse malformed clocklines (was: Re: [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx) Ihor Radchenko
2024-04-14 12:53 ` [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx Ihor Radchenko
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=CH3PR84MB34243D1F564538B01F270DE5C5D42@CH3PR84MB3424.NAMPRD84.PROD.OUTLOOK.COM \
--to=morgan.j.smith@outlook.com \
--cc=emacs-orgmode@gnu.org \
--cc=sanelz@gmail.com \
--cc=yantar92@posteo.net \
/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).