emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Thorsten Jolitz <tjolitz@gmail.com>
Cc: emacs-orgmode@gnu.org
Subject: Re: [RFC] Rewrite `org-entry-properties' using parser
Date: Fri, 01 Aug 2014 15:28:40 +0200	[thread overview]
Message-ID: <87a97o9w53.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <87tx5xunas.fsf@gmail.com> (Thorsten Jolitz's message of "Fri, 01 Aug 2014 01:21:47 +0200")

Hello,

Thorsten Jolitz <tjolitz@gmail.com> writes:

> here is my first take of rewriting `org-entry-properties'.

Interesting. A first round of comments follows.

> Implementation goals were:
>
>  1. (almost) full backward-compability. The parser upcases user
>    properties, thus case-sensitivity is lost after parsing and old
>    applications that rely on the difference between "foo", "Foo" and
>    "FOO" as property keys will break

Case sensitivity never was a feature for properties.

>  - Are some options useless?

Yes, see below.

>  - Is property-classification consistent (e.g. "TODO" in
>    `org-special-properties', but :todo-keyword and :todo-type in the
>    parse-tree)?

"TODO" and :todo-keyword properties are the same.

>  - I actually reimplemented the docstring of the old function instead of
>    the rather complicated code - did I get the semantics right?
>
>  - are there bugs?
>
>  - etc ...
>
> Here is an Org file with the new version of `org-entry-properties',
> helper functions and some 20 ERT-tests. Please have a look.
>
>
> * org-entry-properties
> ** new function (org.el)
>
> #+begin_src emacs-lisp
> (defun org-entry-properties (&optional pom which specific)
>   "Get all properties of the entry at point-or-marker POM.
>
> This includes the TODO keyword, the tags, time strings for
> deadline, scheduled, and clocking, and any additional properties
> defined in the entry.
>
> The return value is an alist, except if WHICH has value `parser',
> then a plist filtered for the properties set by
> `org-element-parse-headline' is returned.

Properties returned by `org-element-at-point' are not meant to be
accessed through `org-entry-properties'. You can remove this option.

>  - nil or `all' :: get all regular (non parser) properties
>
>  - `special' :: get properties that are member of
>    `org-special-properties'
>
>  - `standard' :: get properties of that subclass
>
>  - `parser' :: get properties set by parser (as plist)

See above.

>  - `custom' :: get properties that are member of
>    `org-custom-properties'
>
>  - `default' :: get properties that are member of
>    `org-default-properties'

`org-default-properties' is a (broken) hard-coded list of properties,
only useful for completion. Since this is arbitrary, there's no reason
to check this.

>  - `document' :: get properties that are member of
>    `org-element-document-properties'

The name is misleading, "document properties" are keywords (e.g.
"#+TITLE:"). You can omit this.

>  - `file' :: get properties that are member of
>    `org-file-properties'
>
>  - `global' :: get properties that are member of
>    `org-global-properties'
>
>  - `global-fixed' :: get properties that are member of
>    `org-global-properties-fixed'

The only purpose of `org-global-properties-fixed' is to have a basis for
`org-global-properties'. You can ignore the former.

>  - `non-org' :: get properties that are not member of any of the
>    preceeding classes (except `all')
>
>  - any string :: get only exactly this property
>
>  - form :: get properties string-matched by (rx-to-string form),
>            with FORM being a regular expression in sexp form

Why do you impose `rx-to-string' on the user? Just ask for a regexp. If
one wants exactly a property, he will use `regexp-quote'.

> SPECIFIC can be a string, symbol or keyword, all types will be

keyword type is not needed if you don't access to parser properties.

> converted to an upcased string. It is the specific property we
> are interested in. This argument only exists for historical
> reasons and backward portability, since giving a string value to
> WHICH has the same effect as giving a value to SPECIFIC. However,
> if SPECIFIC is non-nil, it takes precedence over WHICH."
>   (setq which (or which 'all))
>   (org-with-wide-buffer
>    (org-with-point-at pom

`org-with-point-at' already call `org-with-wide-buffer', so you can
remove the latter.

>      (when (and (derived-mode-p 'org-mode)
> 		(ignore-errors (org-back-to-heading t)))
>        (let ((elem (org-element-at-point)))
> 	 (when (eq (car elem) 'headline)

Use `org-element-type', not `car'. Also, you need to include
`inlinetask'

  (when (memq (org-element-type elem) '(headline inlinetask))
    ...)

> 	   (let* ((specific-prop
> 		   (cond
> 		    ((or (org-string-nw-p specific)
> 			 (org-string-nw-p which))
> 		     (upcase
> 		      (or (org-string-nw-p specific)
> 			  (org-string-nw-p which))))
> 		    ((keywordp specific)
> 		     (car (org-split-string
> 			   (format "%s" specific) ":")))

You can ignore `keywordp' part.

> 		    ((and (not (booleanp specific))
> 			  (symbolp specific))
> 		     (upcase (format "%s" specific)))
> 		    (t nil)))

This begs for refactoring. I think

  (specific-prop
   (let ((prop (or specific which)))
     (and prop (upcase (if (stringp prop) prop (symbol-name prop))))))

is sufficient.

> 		  (props-plist (cadr elem))
> 		  (props-alist-strg-keys
> 		   (org-plist-to-alist props-plist nil t))

You shouldn't build this alist each time. More on this below.

> 		  (parser-keywords
> 		   (list :raw-value :title :alt-title :begin :end
> 			 :pre-blank :post-blank :contents-begin
> 			 :contents-end :level :priority :tags
> 			 :todo-keyword :todo-type :scheduled
> 			 :deadline :closed :archivedp :commentedp
> 			 :footnote-section-p))
> 		  (parser-keywords-but-special-props
> 		   (list :raw-value :title :alt-title :begin :end
> 			 :pre-blank :post-blank :contents-begin
> 			 :contents-end :level :archivedp
> 			 :commentedp :footnote-section-p))

This can be ignored.

> 		  ;; FIXME necessary?
> 		  ;; for backward compability only
> 		  (excluded
> 		   '("TODO" "TAGS" "ALLTAGS" "PRIORITY" "BLOCKED"))
> 		  sym)
> 	     (if specific-prop
> 		 (assoc specific-prop props-alist-strg-keys)

THEN branch is not right, as SPECIFIC-PROP could contain a regexp.

> 	       (let ((all-but-parser
> 		      (progn
> 			(setplist 'sym props-plist)
> 			(mapc (lambda (--kw)
> 				(remprop 'sym --kw))
> 			      parser-keywords-but-special-props)
> 			(org-plist-to-alist
> 			 (symbol-plist 'sym) nil t)))

Please ignore the parser part. Also, there's no reason to use a plist
associated to a symbol (i.e., `setplist').

BTW, the algorithm doesn't sound right. You are retrieving all values
from all properties, and then filter out those you do not need. It would
be simpler to list the properties you need and extract their value. No
need to build an alist or juggle with `org-plist-to-alist' then.

Therefore, I suggest to create an alist, as a defconst, e.g.,
`org-internal-properties-alist', that will contain associations between
an internal property name and its key

  '(("TODO" . :todo-keyword)
    ("TAGS" . :tags)
    ...)

When you want to extract a value from the element, you first check this
constant and, if that fails, you build the key out of the name:

 (or (cdr (assoc-ignore-case current-property org-internal-properties-alist))
     (intern (concat ":" current-property)))

> 		     (downcased-special-props
> 		      (mapcar 'downcase org-special-properties)))

Not useful, you can ignore it too: there are `member-ignore-case' and
`assoc-ignore-case'.

> 		 (case which

`case' keys need not be quoted:

  (case which
    (all all-but-parser)
    ...)

> 		   ('all all-but-parser)
> 		   ('non-org (remove-if
> 			      (lambda (--item)
> 				(member
> 				 (car --item)
> 				 (append
> 				  org-special-properties
> 				  org-default-properties
> 				  org-custom-properties
> 				  org-element-document-properties
> 				  org-global-properties
> 				  org-global-properties-fixed
> 				  org-file-properties)))
> 			      all-but-parser))
> 		   ;; FIXME necessary?
> 		   ;; for backward compability only
> 		   ('standard (remove-if
> 			       (lambda (--item)
> 				 (member (car --item) excluded))
> 				 all-but-parser))
> 		   ;; return plist
> 		   ('parser (setplist 'sym props-plist)
> 			    (mapc (lambda (--kw)
> 				    (remprop 'sym --kw))
> 				  (set-difference
> 				   (org-plist-keys props-plist t)
> 				   parser-keywords))
> 			    (symbol-plist 'sym))
> 		   ('special (remove-if-not
> 			      (lambda (--item)
> 				(member
> 				 (car --item)
> 				 downcased-special-props))
> 			      props-alist-strg-keys))
> 		   ('default (remove-if-not
> 			      (lambda (--item)
> 				(member (car --item)
> 					org-default-properties))
> 			      props-alist-strg-keys))
> 		   ('custom (remove-if-not
> 			     (lambda (--item)
> 			       (member (car --item)
> 				       org-custom-properties))
> 			     props-alist-strg-keys))
> 		   ('document (remove-if-not
> 			       (lambda (--item)
> 				 (member
> 				  (car --item)
> 				  org-element-document-properties))
> 			       props-alist-strg-keys))
> 		   ('global (remove-if-not
> 			     (lambda (--item)
> 			       (member (car --item)
> 				       org-global-properties))
> 			     props-alist-strg-keys))
> 		   ('global-fixed (remove-if-not
> 				   (lambda (--item)
> 				     (assoc
> 				      (car --item)
> 				      org-global-properties-fixed))
> 				   props-alist-strg-keys))
> 		   ('file (remove-if-not
> 			   (lambda (--item)
> 			     (member (car --item)
> 				     org-file-properties))
> 			   props-alist-strg-keys))
> 		   (t (when (consp which)
> 			(ignore-errors
> 			  (let ((rgxp (rx-to-string which)))
> 			    (remove-if-not
> 			     (lambda (--item)
> 			       (string-match rgxp (car --item)))
> 			     all-but-parser)))))))))))))))

Since you only need to extract values, you need not use `remove-if-not':

  (let ((predicate
         (case which
           (all #'identity)
           (special (lambda (p) (member-ignore-case p org-special-properties)))
           (default (lambda (p) (member p org-default-properties)))
           ...
           (otherwise (lambda (p) (org-string-match-p which p))))))
    (mapcar (lambda (p)
              (when (funcall predicate p)
                (cons p
                      (org-element-property
                       (or (cdr (assoc-ignore-case current-property
                                                   org-internal-properties-alist))
                           (intern (concat ":" current-property)))
                       elem))))
            list-of-all-properties))

> ** helper function (org-macs.el)

None of them is needed actually.

> ** tests (test-org.el)
>
>
> #+begin_src emacs-lisp
> ;;; Properties
>
> (defconst test-org/org-entry-properties-temp-text
> "* DONE [#A] headline <2014-07-31 Do> :tag:
>   DEADLINE: <2014-08-01 Fr 08:00>
>   - State \"DONE\"       from \"WAITING\"    [2014-07-31 Do 22:45]
>   - State \"WAITING\"    from \"TODO\"       [2014-07-31 Do 14:46] \\
>     testing
>   :PROPERTIES:
>   :CATEGORY: mycat
>   :VISIBILITY_ALL: folded children all
>   :foo-key1: val1
>   :foo-key2: val2
>   :ID:       3996b55d-d678-43a4-af1f-48ed22b5f414
>   :CUSTOM_ID: abc123
>   :bar:      loo
>   :END:
>   [2014-07-31 Do 14:45]
> "
>  "Headline used to test `org-entry-properties'.")
>
> (ert-deftest test-org/org-entry-properties-1 ()
>   "Test of `org-entry-properties' specifications."
>   (should
>   (equal '(("priority" . 65) ("tags" "tag") ("todo-keyword" . "DONE") ("todo-type" . done) ("deadline" timestamp (:type active :raw-value "<2014-08-01 Fr 08:00>" :year-start 2014 :month-start 8 :day-start 1 :hour-start 8 :minute-start 0 :year-end 2014 :month-end 8 :day-end 1 :hour-end 8 :minute-end 0 :begin 56 :end 77 :post-blank 0)) ("CATEGORY" . "mycat") ("VISIBILITY_ALL" . "folded children all") ("FOO-KEY1" . "val1") ("FOO-KEY2" . "val2") ("ID" . "3996b55d-d678-43a4-af1f-48ed22b5f414") ("CUSTOM_ID" . "abc123") ("BAR" . "loo"))
> 	  (org-test-with-temp-text
> 	      test-org/org-entry-properties-temp-text
> 	    (org-entry-properties nil nil nil)))))

I suggest to focus on minimal tests, since those are easier to debug.
IOW, do not load an unwieldy properties drawer but write the particular
part you want to test.


[...]

> (ert-deftest test-org/org-entry-properties-3 ()
>   "Test 3 of `org-entry-properties' specifications."
>   (should
>    (equal nil
> 	  (org-test-with-temp-text
> 	      test-org/org-entry-properties-temp-text
> 	    (org-entry-properties nil nil 'foo)))))

`should-not' is better than `should' + (equal nil ...)

Note that implementation doesn't handle CLOCK, CLOCKSUM, CLOCKSUM_T,
TIMESTAMP, TIMESTAMP_IA, and BLOCKED values yet.


Regards,

-- 
Nicolas Goaziou

  parent reply	other threads:[~2014-08-01 13:28 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-31 23:21 [RFC] Rewrite `org-entry-properties' using parser Thorsten Jolitz
2014-08-01 11:44 ` Bastien
2014-08-01 12:44   ` Thorsten Jolitz
2014-08-01 13:28 ` Nicolas Goaziou [this message]
2014-08-05 12:52   ` Thorsten Jolitz
2014-08-06  9:59     ` Rasmus
2014-08-06 12:04     ` Nicolas Goaziou
2014-08-21  0:16       ` Thorsten Jolitz
2014-08-01 18:41 ` Erik Hetzner
2014-08-03 18:59   ` John Kitchin
2014-08-04  3:45     ` Erik Hetzner
2014-08-04  4:07       ` Aaron Ecay
2014-08-04 14:11         ` Erik Hetzner

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=87a97o9w53.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=emacs-orgmode@gnu.org \
    --cc=tjolitz@gmail.com \
    /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).