* [BUG] colview.el regexp - capture operator when title is empty
@ 2024-08-19 10:45 Sławomir Grochowski
2024-08-20 18:21 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: Sławomir Grochowski @ 2024-08-19 10:45 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 82 bytes --]
Hello,
I have found a bug in regexp.
Test that need to pass to fix it is below.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-testing-lisp-test-org-colview.el-add-test-to-pass-to.patch --]
[-- Type: text/x-diff, Size: 1049 bytes --]
From 9710f6befb1bd4eee2d508d7baaee95e825a68dc Mon Sep 17 00:00:00 2001
From: Slawomir Grochowski <slawomir.grochowski@gmail.com>
Date: Mon, 19 Aug 2024 12:30:49 +0200
Subject: [PATCH] testing/lisp/test-org-colview.el: add test to pass to fix a
bug
---
testing/lisp/test-org-colview.el | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/testing/lisp/test-org-colview.el b/testing/lisp/test-org-colview.el
index 6b603c31b..8615ef760 100644
--- a/testing/lisp/test-org-colview.el
+++ b/testing/lisp/test-org-colview.el
@@ -90,7 +90,12 @@
(should
(equal `(("ITEM" "ITEM" nil "+" "%.1f"))
(org-columns-compile-format
- "%ITEM{+;%.1f}"))))
+ "%ITEM{+;%.1f}")))
+ (should
+ ;; Bug - when the title is empty, the regular expression will not capture the operator.
+ (equal '(("ITEM" "ITEM" nil "X" nil))
+ (org-columns-compile-format
+ "%ITEM(){X}"))))
(ert-deftest test-org-colview/substring-below-width ()
"Test `org-columns--truncate-below-width'."
--
2.30.2
[-- Attachment #3: Type: text/plain, Size: 223 bytes --]
Steps to reproduce:
1. add new column with an empty title and select one of the operators.
2. press 's' when the cursor is the added column - operator should be
selected, but it's empty.
Regards,
--
Slawomir Grochowski
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] colview.el regexp - capture operator when title is empty
2024-08-19 10:45 [BUG] colview.el regexp - capture operator when title is empty Sławomir Grochowski
@ 2024-08-20 18:21 ` Ihor Radchenko
2024-08-21 8:27 ` Sławomir Grochowski
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2024-08-20 18:21 UTC (permalink / raw)
To: Sławomir Grochowski; +Cc: emacs-orgmode
Sławomir Grochowski <slawomir.grochowski@gmail.com> writes:
> I have found a bug in regexp.
> Test that need to pass to fix it is below.
Thanks!
May you also fix the regexp in `org-columns-compile-format'?
I recommend using `rx' to make the regexp more readable and less prone
to mistakes.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] colview.el regexp - capture operator when title is empty
2024-08-20 18:21 ` Ihor Radchenko
@ 2024-08-21 8:27 ` Sławomir Grochowski
2024-08-25 12:35 ` Sławomir Grochowski
0 siblings, 1 reply; 7+ messages in thread
From: Sławomir Grochowski @ 2024-08-21 8:27 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode
Ihor Radchenko <yantar92@posteo.net> writes:
> May you also fix the regexp in `org-columns-compile-format'?
> I recommend using `rx' to make the regexp more readable and less prone
> to mistakes.
OK. I will try. String-based Emacs regexp is unreadable and quite scary
for me. That's why I always convert all regexes to the `rx' form. But I
thought that since it's just a small error, maybe someone proficient in
string-based Emacs regexp can easily fix it. Because when transferring
it to the `rx' form, I may introduce more errors. But we have some tests
so I will try to do it this way.
--
Slawomir Grochowski
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] colview.el regexp - capture operator when title is empty
2024-08-21 8:27 ` Sławomir Grochowski
@ 2024-08-25 12:35 ` Sławomir Grochowski
2024-08-25 12:39 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: Sławomir Grochowski @ 2024-08-25 12:35 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode
I tried to describe the problem better as well as my thought process:
The `org-columns-compile-format' function has a bug in regexp.
When an empty parentheses `()` is in the 'column format string' -
and it always is when user do not provide a column title - then the
regexp can't capture the operator which is in curly brackets `{}`.
Try yourself with code below:
#+begin_src elisp
(org-columns-compile-format"%ITEM(){operator}")
; return => (("ITEM" "ITEM" nil nil nil))
; should be => (("ITEM" "ITEM" nil operator nil))
#+end_src
I'm not very skilled in regex, so following Ihor's advice I decided to
write the regex in the form of 'rx' which is more readable. Below is
the modified function:
#+begin_src elisp
(defun org-columns-compile-format-rx (fmt)
"Turn a column format string FMT into an alist of specifications.
The alist has one entry for each column in the format. The elements of
that list are:
property the property name, as an upper-case string
title the title field for the columns, as a string
width the column width in characters, can be nil for automatic width
operator the summary operator, as a string, or nil
printf a printf format for computed values, as a string, or nil
This function updates `org-columns-current-fmt-compiled'."
(setq org-columns-current-fmt-compiled nil)
(let ((start 0))
(while (string-match
(rx "%"
(optional (group (+ digit)))
(group (one-or-more (in alnum "_-")))
(optional "(" (group (one-or-more (not (any ")")))) ")")
(optional "{" (group (one-or-more (not (any "}")))) "}")
(zero-or-more space))
fmt start)
(setq start (match-end 0))
(let* ((width (and (match-end 1) (string-to-number (match-string 1 fmt))))
(prop (match-string-no-properties 2 fmt))
(title (or (match-string-no-properties 3 fmt) prop))
(operator (match-string-no-properties 4 fmt)))
(push (if (not operator) (list (upcase prop) title width nil nil)
(let (printf)
(when (string-match ";" operator)
(setq printf (substring operator (match-end 0)))
(setq operator (substring operator 0 (match-beginning 0))))
(list (upcase prop) title width operator printf)))
org-columns-current-fmt-compiled)))
(setq org-columns-current-fmt-compiled
(nreverse org-columns-current-fmt-compiled))))
#+end_src
I am checking if in this particular case it returns the same result
as 'org-columns-compile-format':
#+begin_src elisp
(org-columns-compile-format-rx "%ITEM(){operator}") ;; => (("ITEM" "ITEM" nil nil nil))
#+end_src
#+begin_src elisp
(equal
(org-columns-compile-format "%ITEM(){operator}")
(org-columns-compile-format-rx "%ITEM(){operator}")) ;; => t
#+end_src
Yes, in this particular case it works the same. To make it easier to
capture the error, I only take a piece of code responsible for the
regexp:
#+begin_src elisp
(let ((text "%25ITEM(){operator}")
(pattern (rx "%"
(optional (group (+ digit)))
(group (one-or-more (in alnum "_-")))
(optional "(" (group (one-or-more (not (any ")")))) ")")
(optional "{" (group (one-or-more (not (any "}")))) "}")
(zero-or-more space))))
(if (string-match pattern text)
(mapcar (lambda (i) (match-string i text))
(number-sequence 0 (/ (length (match-data)) 2)))))
;=> ("%25ITEM" "25" "ITEM" nil)
#+end_src
Through trial and error, I came to a solution that provides correct
results. I changed the expression 'one-or-more' to 'zero-or-more' for
'title' and 'operator':
#+begin_src elisp
(let ((text "%25ITEM(){operator}")
(pattern (rx "%"
(optional (group (+ digit)))
(group (one-or-more (in alnum "_-")))
(optional "(" (group (zero-or-more (not (any ")")))) ")")
(optional "{" (group (zero-or-more (not (any "}")))) "}")
(zero-or-more space))))
(if (string-match pattern text)
(mapcar (lambda (i) (match-string i text))
(number-sequence 0 (/ (length (match-data)) 2)))))
;=> ("%25ITEM(){operator}" "25" "ITEM" "" "operator" nil)
#+end_src
This fixed a problem but also changed the return value. Now, when we
have empty parentheses '()', it will return an empty string
instead of null.
Therefore, in the function `org-columns-compile-format`, I added calls
to `org-string-nw-p` for the variables 'title' and 'operator'.
Additionally, I think that adding empty parentheses '()' should be removed
when we do not specify a 'column title'. Because it does not provide
any value. So I added another call to `org-string-nw-p' in function
`org-columns-new'.
Patch below:
<#part type="text/x-diff" filename="~/.emacs.d/straight/repos/org/0001-lisp-org-colview.el-org-columns-compile-format-regex.patch" disposition=inline>
<#/part>
Regards,
--
Slawomir Grochowski
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] colview.el regexp - capture operator when title is empty
2024-08-25 12:35 ` Sławomir Grochowski
@ 2024-08-25 12:39 ` Ihor Radchenko
2024-08-25 12:59 ` Sławomir Grochowski
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2024-08-25 12:39 UTC (permalink / raw)
To: Sławomir Grochowski; +Cc: emacs-orgmode
Sławomir Grochowski <slawomir.grochowski@gmail.com> writes:
> Patch below:
> <#part type="text/x-diff" filename="~/.emacs.d/straight/repos/org/0001-lisp-org-colview.el-org-columns-compile-format-regex.patch" disposition=inline>
> <#/part>
There is no patch attached.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] colview.el regexp - capture operator when title is empty
2024-08-25 12:39 ` Ihor Radchenko
@ 2024-08-25 12:59 ` Sławomir Grochowski
2024-08-31 13:34 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: Sławomir Grochowski @ 2024-08-25 12:59 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 76 bytes --]
Ihor Radchenko <yantar92@posteo.net> writes:
> There is no patch attached.
[-- Attachment #2: 0001-lisp-org-colview.el-org-columns-compile-format-regex.patch --]
[-- Type: text/x-diff, Size: 3702 bytes --]
From 987f66774db4ad1e515cac4b9fc5ba46bd627c27 Mon Sep 17 00:00:00 2001
From: Slawomir Grochowski <slawomir.grochowski@gmail.com>
Date: Sat, 24 Aug 2024 18:47:09 +0200
Subject: [PATCH] lisp/org-colview.el: org-columns-compile-format regexp bugfix
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lisp/org-colview.el (org-columns-compile-format): Refactor
string-based regexp to `rx' form. Fix regexp: When an empty
parentheses `()` is in the column format string then the regexp
can't capture the operator which is in curly brackets `{}`.
(org-columns-new): prevent adding empty parantheses '()' to 'column
format string' when 'column title' is empty.
* testing/lisp.test-org-colview.el (test-org-colview/compile-format):
add new case to test: check if operator will be captured when there is
empty parantheses '()' in 'column format string'.
Reported-by: Sławomir Grochowski <slawomir.grochowski@gmail.com>
Link: https://list.orgmode.org/orgmode/877ccczt83.fsf@gmail.com/
---
lisp/org-colview.el | 19 ++++++++++++-------
testing/lisp/test-org-colview.el | 7 ++++++-
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/lisp/org-colview.el b/lisp/org-colview.el
index bc93941e4..b111c1676 100644
--- a/lisp/org-colview.el
+++ b/lisp/org-colview.el
@@ -935,8 +935,9 @@ details."
(mapcar #'list (org-buffer-property-keys t nil t))
nil nil (nth 0 spec))))
(list prop
- (read-string (format "Column title [%s]: " prop)
- (nth 1 spec))
+ (org-string-nw-p
+ (read-string (format "Column title [%s]: " prop)
+ (nth 1 spec)))
;; Use `read-string' instead of `read-number'
;; to allow empty width.
(let ((w (read-string
@@ -1183,14 +1184,18 @@ This function updates `org-columns-current-fmt-compiled'."
(setq org-columns-current-fmt-compiled nil)
(let ((start 0))
(while (string-match
- "%\\([0-9]+\\)?\\([[:alnum:]_-]+\\)\\(?:(\\([^)]+\\))\\)?\
-\\(?:{\\([^}]+\\)}\\)?\\s-*"
- fmt start)
+ (rx "%"
+ (optional (group (+ digit)))
+ (group (one-or-more (in alnum "_-")))
+ (optional "(" (group (zero-or-more (not (any ")")))) ")")
+ (optional "{" (group (zero-or-more (not (any "}")))) "}")
+ (zero-or-more space))
+ fmt start)
(setq start (match-end 0))
(let* ((width (and (match-end 1) (string-to-number (match-string 1 fmt))))
(prop (match-string-no-properties 2 fmt))
- (title (or (match-string-no-properties 3 fmt) prop))
- (operator (match-string-no-properties 4 fmt)))
+ (title (or (org-string-nw-p (match-string-no-properties 3 fmt)) prop))
+ (operator (org-string-nw-p (match-string-no-properties 4 fmt))))
(push (if (not operator) (list (upcase prop) title width nil nil)
(let (printf)
(when (string-match ";" operator)
diff --git a/testing/lisp/test-org-colview.el b/testing/lisp/test-org-colview.el
index 6b603c31b..623ee6283 100644
--- a/testing/lisp/test-org-colview.el
+++ b/testing/lisp/test-org-colview.el
@@ -90,7 +90,12 @@
(should
(equal `(("ITEM" "ITEM" nil "+" "%.1f"))
(org-columns-compile-format
- "%ITEM{+;%.1f}"))))
+ "%ITEM{+;%.1f}")))
+ (should
+ ;; Bug https://list.orgmode.org/orgmode/877ccczt83.fsf@gmail.com/
+ (equal '(("ITEM" "ITEM" nil "X" nil))
+ (org-columns-compile-format
+ "%ITEM(){X}"))))
(ert-deftest test-org-colview/substring-below-width ()
"Test `org-columns--truncate-below-width'."
--
2.30.2
[-- Attachment #3: Type: text/plain, Size: 25 bytes --]
--
Slawomir Grochowski
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-31 13:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-19 10:45 [BUG] colview.el regexp - capture operator when title is empty Sławomir Grochowski
2024-08-20 18:21 ` Ihor Radchenko
2024-08-21 8:27 ` Sławomir Grochowski
2024-08-25 12:35 ` Sławomir Grochowski
2024-08-25 12:39 ` Ihor Radchenko
2024-08-25 12:59 ` Sławomir Grochowski
2024-08-31 13:34 ` Ihor Radchenko
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).