From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [RFC] Rewrite `org-entry-properties' using parser Date: Fri, 01 Aug 2014 15:28:40 +0200 Message-ID: <87a97o9w53.fsf@nicolasgoaziou.fr> References: <87tx5xunas.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:37482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDCsT-0004TB-JI for emacs-orgmode@gnu.org; Fri, 01 Aug 2014 09:28:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XDCsJ-00080A-Ck for emacs-orgmode@gnu.org; Fri, 01 Aug 2014 09:28:09 -0400 Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:46118) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDCsJ-0007zQ-2N for emacs-orgmode@gnu.org; Fri, 01 Aug 2014 09:27:59 -0400 In-Reply-To: <87tx5xunas.fsf@gmail.com> (Thorsten Jolitz's message of "Fri, 01 Aug 2014 01:21:47 +0200") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Thorsten Jolitz Cc: emacs-orgmode@gnu.org Hello, Thorsten Jolitz 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