Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Kyle Meyer writes:
> Nick Dokos writes:
>
>> Here's the amended patch: it includes the fixes from Kyle's review, the
>> modification
>> he suggested that adds the plain property for each _ALL property to the list
>> and a few test cases to the test/org-buffer-property-keys test.
>
> Thank you for the updates. Applied (bc4fa8a00).
>
>> -nil)))
>> +;; Get property names from #+PROPERTY keywords as well
>> +(mapcar (lambda (s)
>> + (nth 0 (split-string s)))
>> +(cdar (org-collect-keywords '("PROPERTY"
>> +nil))
>
> I didn't spot it earlier, but this nil (not added by your patch) is
> unnecessary. Since the patch is touching the line anyway, I've dropped
> it on apply.
>
>> +bare-props)
>> (org-with-wide-buffer
>> (goto-char (point-min))
>> (while (re-search-forward org-property-start-re nil t)
>> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>> (let ((p (match-string-no-properties 1 value)))
>> (unless (member-ignore-case p org-special-properties)
>> (push p props))
>> -(sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase
>> b))
>> +(sort (delete-dups (append props
>> + ;; for each xxx_ALL property, make sure
>> + ;; the bare xxx property is also
>> + ;; included
>> + (dolist (x props bare-props)
>> + (if (string-match "_ALL\\b" x)
>> + (setq bare-props (cons (substring x 0 -4)
>> +bare-props))
>
> I did a cosmetic rewrite here to use mapcar, which I hope you won't
> mind.
>
BTW, I just thought of a possible problem: the manual says that property
keys are case-insensitive (although all the examples I can find spell
"_ALL" in upper case, but if I write
:PROPERTIES:
:foo_all: bar baz
:END:
I don't think that the code is going to handle it correctly. There
are other places that also use "_ALL" without a let of case-fold-search
(at least AFAICT).
Am I paranoid or is this a problem?
--
Nick
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Kyle Meyer writes:
> Nick Dokos writes:
>
>> Here's the amended patch: it includes the fixes from Kyle's review, the
>> modification
>> he suggested that adds the plain property for each _ALL property to the list
>> and a few test cases to the test/org-buffer-property-keys test.
>
> Thank you for the updates. Applied (bc4fa8a00).
>
>> -nil)))
>> +;; Get property names from #+PROPERTY keywords as well
>> +(mapcar (lambda (s)
>> + (nth 0 (split-string s)))
>> +(cdar (org-collect-keywords '("PROPERTY"
>> +nil))
>
> I didn't spot it earlier, but this nil (not added by your patch) is
> unnecessary. Since the patch is touching the line anyway, I've dropped
> it on apply.
>
>> +bare-props)
>> (org-with-wide-buffer
>> (goto-char (point-min))
>> (while (re-search-forward org-property-start-re nil t)
>> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>> (let ((p (match-string-no-properties 1 value)))
>> (unless (member-ignore-case p org-special-properties)
>> (push p props))
>> -(sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase
>> b))
>> +(sort (delete-dups (append props
>> + ;; for each xxx_ALL property, make sure
>> + ;; the bare xxx property is also
>> + ;; included
>> + (dolist (x props bare-props)
>> + (if (string-match "_ALL\\b" x)
>> + (setq bare-props (cons (substring x 0 -4)
>> +bare-props))
>
> I did a cosmetic rewrite here to use mapcar, which I hope you won't
> mind.
Not at all - thanks!
>
> Thanks again.
>
>
--
Nick
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Nick Dokos writes:
> Here's the amended patch: it includes the fixes from Kyle's review, the
> modification
> he suggested that adds the plain property for each _ALL property to the list
> and a few test cases to the test/org-buffer-property-keys test.
Thank you for the updates. Applied (bc4fa8a00).
> - nil)))
> + ;; Get property names from #+PROPERTY keywords as well
> + (mapcar (lambda (s)
> + (nth 0 (split-string s)))
> + (cdar (org-collect-keywords '("PROPERTY"
> + nil))
I didn't spot it earlier, but this nil (not added by your patch) is
unnecessary. Since the patch is touching the line anyway, I've dropped
it on apply.
> + bare-props)
> (org-with-wide-buffer
> (goto-char (point-min))
> (while (re-search-forward org-property-start-re nil t)
> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>(let ((p (match-string-no-properties 1 value)))
> (unless (member-ignore-case p org-special-properties)
>(push p props))
> -(sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase
> b))
> +(sort (delete-dups (append props
> +;; for each xxx_ALL property, make sure
> +;; the bare xxx property is also
> +;; included
> +(dolist (x props bare-props)
> + (if (string-match "_ALL\\b" x)
> + (setq bare-props (cons (substring x 0 -4)
> + bare-props))
I did a cosmetic rewrite here to use mapcar, which I hope you won't
mind.
Thanks again.
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Here's the amended patch: it includes the fixes from Kyle's review, the
modification
he suggested that adds the plain property for each _ALL property to the list
and a few test cases to the test/org-buffer-property-keys test.
--8<---cut here---start->8---
>From 60b9ababe42c91ec6fcd2c53f6923d75daa12454 Mon Sep 17 00:00:00 2001
From: Nick Dokos
Date: Mon, 6 Jul 2020 21:49:41 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
completion list
* lisp/org.el (org-buffer-property-keys): Enhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers. Also, for each xxx_ALL property, make sure that the bare xxx
property is added too.
* testing/lisp/test-org.el (test-org/buffer-property-keys): Add test
cases for #+PROPERTY keywords and also for xxx_ALL --> xxx properties.
See https://emacs.stackexchange.com/questions/59448/ for details.
---
lisp/org.el | 17 +++--
testing/lisp/test-org.el | 21 +
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index 4a1a83d0f..ccdb0dd5c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,7 +13084,12 @@ COLUMN formats in the current buffer."
(props (append
(and specials org-special-properties)
(and defaults (cons org-effort-property org-default-properties))
- nil)))
+ ;; Get property names from #+PROPERTY keywords as well
+ (mapcar (lambda (s)
+ (nth 0 (split-string s)))
+ (cdar (org-collect-keywords '("PROPERTY"
+ nil))
+ bare-props)
(org-with-wide-buffer
(goto-char (point-min))
(while (re-search-forward org-property-start-re nil t)
@@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
(let ((p (match-string-no-properties 1 value)))
(unless (member-ignore-case p org-special-properties)
(push p props))
-(sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase b))
+(sort (delete-dups (append props
+ ;; for each xxx_ALL property, make sure
+ ;; the bare xxx property is also
+ ;; included
+ (dolist (x props bare-props)
+(if (string-match "_ALL\\b" x)
+(setq bare-props (cons (substring x 0 -4)
+ bare-props))
+ (lambda (a b) (string< (upcase a) (upcase b))
(defun org-property-values (key)
"List all non-nil values of property KEY in current buffer."
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 30a8067db..61e642d4f 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -5409,6 +5409,27 @@ Paragraph"
(equal '("A")
(org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
(org-buffer-property-keys
+ ;; Add bare property if xxx_ALL property is there
+ (should
+ (equal '("A" "B" "B_ALL")
+ (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:B_ALL:
foo bar\n:END:"
+ (org-buffer-property-keys
+ ;; Add bare property if xxx_ALL property is there - check dupes
+ (should
+ (equal '("A" "B" "B_ALL")
+ (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:B: 2\n:B_ALL:
foo bar\n:END:"
+ (org-buffer-property-keys
+ ;; Retrieve properties from #+PROPERTY keyword lines
+ (should
+ (equal '("A" "C")
+ (org-test-with-temp-text "#+PROPERTY: C foo\n* H\n:PROPERTIES:\n:A:
1\n:A+: 2\n:END:"
+ (org-buffer-property-keys
+ ;; Retrieve properties from #+PROPERTY keyword lines - make sure an _ALL
property also
+ ;; adds the bare property
+ (should
+ (equal '("A" "C" "C_ALL")
+ (org-test-with-temp-text "#+PROPERTY: C_ALL foo bar\n*
H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
+ (org-buffer-property-keys
;; With non-nil COLUMNS, extract property names from columns.
(should
(equal '("A" "B")
--
2.25.1
--8<---cut here---end--->8---
--
Nick
"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Kyle Meyer writes: > ,,, > I think this patch is a clear improvement as is, but in the context of > completion (and the stack exchange post you link to), isn't the handling > around *_ALL keywords still a bit off? It seems a caller would want to > complete without the _ALL; to use the example from that post, with > "#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE". > Is it worth providing special handling here? > With a bit of caffeine in my system, I now see what you wrote and I think you are right: if the _ALL property is present, then the bare property should be added to the completion list if not already there. -- Nick "There are only two hard problems in computer science: cache invalidation, naming things, and off-by-one errors." -Martin Fowler
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Hi Kyle,
thanks for the review.
Kyle Meyer writes:
> Nick Dokos writes:
>
>> Here's a patch to enhance the property name completion list with names from
>> #+PROPERTY keyword lines: at the moment, only property names found in
>> property
>> drawers are used to populate the completion list.
>
> Thanks for the patch.
>
>> org: add property names from #+PROPERTY keywords to completion list
>>
>> * lisp/org.el (org-buffer-property-keys): ehhance the completion list
>
> Typo: enhance. And as a convention nit, it should be capitalized.
>
Fixed.
>> with property names from #+PROPERTY keywords, not just property
>> drawers.
>>
>> ...
>> +(mapcar (lambda (s)
>> + (let ((split (split-string s)))
>> +(nth 0 split)))
>> +(cdar (org-collect-keywords '("PROPERTY"
>> nil)))
>
> IMO the let-binding doesn't add any clarity over
>
> (nth 0 (split-string s))
>
Fixed.
>
> I wondered about possible duplicates, but it looks like
> org-buffer-property-keys already takes care of that at the end.
>
> I think this patch is a clear improvement as is, but in the context of
> completion (and the stack exchange post you link to), isn't the handling
> around *_ALL keywords still a bit off? It seems a caller would want to
> complete without the _ALL; to use the example from that post, with
> "#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE".
> Is it worth providing special handling here?
>
>
My assumption was that one could put in two properties (that's what
the OP was doing in his setup file): a GENRE_ALL one already properly
set-up and the corresponding bare GENRE property as a placeholder with
an empty value, just to get the completion. It seems to me like a
reasonable way to do it, without making org-buffer-property-keys too
opinionated.
I could see stripping all the _ALL suffixes from the property names
at the end, but personally it just makes me a bit queasy :) But if
there is consensus one way or the other, I'd be happy to implement it.
Here's the updated patch:
--8<---cut here---start->8---
>From 50c625f935d5581952d37801943550ff44c473ff Mon Sep 17 00:00:00 2001
From: Nick Dokos
Date: Mon, 6 Jul 2020 21:49:41 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
completion list
* lisp/org.el (org-buffer-property-keys): Enhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers.
See https://emacs.stackexchange.com/questions/59448/ for details.
---
lisp/org.el | 4
1 file changed, 4 insertions(+)
diff --git a/lisp/org.el b/lisp/org.el
index 4a1a83d0f..8deaa1ed9 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,6 +13084,10 @@ COLUMN formats in the current buffer."
(props (append
(and specials org-special-properties)
(and defaults (cons org-effort-property org-default-properties))
+ ;; Get property names from #+PROPERTY keywords as well
+ (mapcar (lambda (s)
+ (nth 0 (split-string s)))
+ (cdar (org-collect-keywords '("PROPERTY"
nil)))
(org-with-wide-buffer
(goto-char (point-min))
--
2.25.1
--8<---cut here---end--->8---
--
Nick
"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Nick Dokos writes:
> Here's a patch to enhance the property name completion list with names from
> #+PROPERTY keyword lines: at the moment, only property names found in property
> drawers are used to populate the completion list.
Thanks for the patch.
> org: add property names from #+PROPERTY keywords to completion list
>
> * lisp/org.el (org-buffer-property-keys): ehhance the completion list
Typo: enhance. And as a convention nit, it should be capitalized.
> with property names from #+PROPERTY keywords, not just property
> drawers.
>
> See https://emacs.stackexchange.com/questions/59448/ for details.
> ---
> lisp/org.el | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 748c058ca..0e83162e8 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -13084,6 +13084,11 @@ COLUMN formats in the current buffer."
> (props (append
> (and specials org-special-properties)
> (and defaults (cons org-effort-property org-default-properties))
> + ;; Get property names from #+PROPERTY keywords as well
> + (mapcar (lambda (s)
> + (let ((split (split-string s)))
> + (nth 0 split)))
> + (cdar (org-collect-keywords '("PROPERTY"
> nil)))
IMO the let-binding doesn't add any clarity over
(nth 0 (split-string s))
I wondered about possible duplicates, but it looks like
org-buffer-property-keys already takes care of that at the end.
I think this patch is a clear improvement as is, but in the context of
completion (and the stack exchange post you link to), isn't the handling
around *_ALL keywords still a bit off? It seems a caller would want to
complete without the _ALL; to use the example from that post, with
"#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE".
Is it worth providing special handling here?
Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
Adding a simple test to the previous patch:
--8<---cut here---start->8---
>From cae6b5596f69968003c053f53cb45ffb4139a5ad Mon Sep 17 00:00:00 2001
From: Nick Dokos
Date: Mon, 6 Jul 2020 21:07:01 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
completion list
* lisp/org.el (org-buffer-property-keys): ehhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers.
See https://emacs.stackexchange.com/questions/59448/ for details.
---
lisp/org.el | 5 +
testing/lisp/test-org.el | 5 +
2 files changed, 10 insertions(+)
diff --git a/lisp/org.el b/lisp/org.el
index 748c058ca..0e83162e8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,6 +13084,11 @@ COLUMN formats in the current buffer."
(props (append
(and specials org-special-properties)
(and defaults (cons org-effort-property org-default-properties))
+ ;; Get property names from #+PROPERTY keywords as well
+ (mapcar (lambda (s)
+ (let ((split (split-string s)))
+ (nth 0 split)))
+ (cdar (org-collect-keywords '("PROPERTY"
nil)))
(org-with-wide-buffer
(goto-char (point-min))
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 3c563f344..ddda96105 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -5352,6 +5352,11 @@ Paragraph"
(equal '("A")
(org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
(org-buffer-property-keys
+ ;; Retrieve properties from #+PROPERTY keyword lines
+ (should
+ (equal '("A" "C")
+ (org-test-with-temp-text "#+PROPERTY: C foo\n* H\n:PROPERTIES:\n:A:
1\n:A+: 2\n:END:"
+(org-buffer-property-keys
;; With non-nil COLUMNS, extract property names from columns.
(should
(equal '("A" "B")
--
2.25.4
--8<---cut here---end--->8---
--
Nick
"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler
