emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [RFC] Rewrite `org-entry-properties' using parser


From: Nicolas Goaziou
Subject: Re: [O] [RFC] Rewrite `org-entry-properties' using parser
Date: Fri, 01 Aug 2014 15:28:40 +0200

Hello,

Thorsten Jolitz <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]