Re: [O] [RFC] [PATCH] Changes to Tag groups - allow nesting and regexps

[Nicolas Goaziou]

Gustav Wikström <address@hidden> writes:

> Hi again! The FSA-assignment is now complete. New patches are attached
> and comments below.

OK. I updated list of contributors accordingly.

> The reason for the use of [ ] is because { } already has another purpose
> - it is used to make the tags within { } exclusive.
>
> this example
>
> ,----
> | #+TAGS: { group : include1 include2 }
> `----
>
> will only allow one of the tags on any specific headline. [ ] solves
> this. Note that grouptags doesn't care if { } or [ ] is used. The only
> difference is the exclusiveness. I.e both
>
> ,----
> | #+TAGS: [ group : include1 include2 ]
> | #+TAGS: { group : include1 include2 }
> `----
>
> will work. With some limitations on the second example due to the way {
> } works since before.

OK, but is it really needed? What is the point of having two tags of the
same group (or, if we consider nested group tags, the same set of
siblings) at the same time?

> Subject: [PATCH 1/3] org: Grouptags not unique and can contain regexp
>
> * lisp/org.el (org--setup-process-tags):
>   (org-fast-tag-selection):
>
>   Grouptags had to previously be defined with { }. This syntax is
>   already used for exclusive tags and Grouptags need their own,
>   non-exclusive syntax. This behaviour is achieved with [ ]
>   instead. Note: { } can still be used also for Grouptags but then
>   only one of the given tags can be used on the headline at the same
>   time. Example:

You need to separate sentences with two spaces.  Also, commit messages
need to be formatted this way

 * lisp/org.el (function): Small description.
   (other function): Small description.

 Long explanations.

> -        (org-re "\\`\\([[:alnum:address@hidden)\\(?:(\\(.\\))\\)?\\'") e)
> +        (org-re (concat "\\`\\([[:alnum:address@hidden"
> +                        "\\|{.+}\\)" ; regular expression
                               ^^^
I think it should be non-greedy above.

> +          (taggroups (or org-tag-groups-alist-for-agenda 
> org-tag-groups-alist))
> +          (taggroups (if downcased (mapcar (lambda(tg) (mapcar 'downcase 
> tg)) taggroups) taggroups))

Nitpick:

  "(lambda (tg) ... #'downcase ...)"

Also it should be wrapped at 80 characters.

> +          (taggroups-keys (mapcar 'car taggroups))

Nitpick: (mapcar #'car taggroups)

> +          (return-match (if downcased (downcase match) match))
> +          (count 0)
> +          regexps-in-match tags-in-group regexp-in-group 
> regexp-in-group-escaped)
>       ;; @ and _ are allowed as word-components in tags
>       (modify-syntax-entry ?@ "w" stable)
>       (modify-syntax-entry ?_ "w" stable)
> -     (while (and tml
> +     ;; Temporarily replace regexp-expressions in the match-expression

Nitpick: missing full stop.

> +     (while (string-match "{.+?}" return-match)
> +       (setq count (1+ count))

Nitpick:

  (incf count)

> +       (setq regexps-in-match (cons (match-string 0 return-match) 
> regexps-in-match))

Nitpick:

  (push (match-string 0 return-match) regexps-in-match)

> +       (setq return-match (replace-match (concat "<" (number-to-string 
> count) ">") t nil return-match)))

Nitpick:

  (format "<%d>" count)

> +           ; Filter tag-regexps from tags
> +           (setq regexp-in-group-escaped (delq nil (mapcar (lambda (x)
> +                                                             (if (stringp x)
> +                                                                 (and 
> (string-prefix-p "{" x)
> +                                                                      
> (string-suffix-p "}" x)
> +                                                                      x)
> +                                                               x)) 
> tags-in-group))

We cannot use `string-prefix-p' and `string-suffix-p' due to backward
compatibility. The former will be fine in Org 8.4 (it was introduced in
Emacs 24.1), but the latter comes from Emacs 24.4.

You can use (equal (substring x ... ...) ...) instead.

Be careful about line width, too.

> +                 tags-in-group (delq nil (mapcar (lambda (x)
> +                                                   (if (stringp x)
> +                                                       (and (not 
> (string-prefix-p "{" x))
> +                                                            (not 
> (string-suffix-p "}" x))
> +                                                            x)
> +                                                     x)) tags-in-group)))

Ditto.

> +           ; If single-as-list, do no more in the while-loop...
> +           (if (not single-as-list)
> +               (progn
> +                 (if regexp-in-group
> +                     (setq regexp-in-group (concat "\\|" (mapconcat 
> 'identity regexp-in-group "\\|"))))
> +                 (setq tags-in-group (concat dir "{\\<" (regexp-opt 
> tags-in-group) regexp-in-group  "\\>}"))
> +                 (if (stringp tags-in-group) (org-add-props tags-in-group 
> '(grouptag t)))

Nitpick: (when (stringp tags-in-group) ...)

> +                 (setq return-match (replace-match tags-in-group t t 
> return-match)))
> +             (setq tags-in-group (append regexp-in-group-escaped 
> tags-in-group))))
> +         (setq taggroups-keys (delete tag taggroups-keys))))

I think you can refactor this part into a `cond'.

  (cond (single-as-list (setq taggroups-keys (delete tag taggroups-keys)))
        (regexp-in-group (setq regexp-in-group ...))
        (t (setq tags-in-group ....)
           (when (stringp tags-in-group) ...)
           (setq return match ...))

> +     ;; Add the regular expressions back into the match-expression again

See above.

> +     (while regexps-in-match
> +       (setq return-match (replace-regexp-in-string (concat "<" 
> (number-to-string count) ">")

See above.

> +      ((equal (car e) :startgrouptags)

`eq' instead of `equal'.

> +       (setq intaggroup t)
> +       (when (not (= cnt 0))

(unless (zerop cnt) ...)

or

(unless (= cnt 0) ...)

> +      ((equal (car e) :endgrouptags)

See above.

>         (when (not (= cnt 0))

Ditto.

> +       (if (and (= cnt 0) (not ingroup) (not intaggroup)) (insert " "))

`when' instead of `if'.

> +         (if (or ingroup intaggroup) (insert " "))

Ditto.

> Subject: [PATCH 2/3] org-agenda: Filtering in the agenda on grouptags

[...]

> +      (while (not (member char (append '(?\t ?\r ?/ ?. ?\ ?q)
> +                                    (string-to-list tag-chars))))

Nitpick: `memq' instead of `member'.

> +     (cond ((equal char ?-) (setq exclude t))
> +           ((equal char ?+) (setq exclude nil)))))

Nitpick: `eq' instead of `equal'.

> +     ((equal char ?q)) ;If q, abort (even if there is a q-key for a tag...)

Ditto.

> -(defun org-agenda-filter-make-matcher (filter type)
> +(defun org-agenda-filter-make-matcher (filter type &optional expand)
>    "Create the form that tests a line for agenda filter."

You need to explain arguments (data type, purpose) in the docstring.

> +      ;(if expand (setq filter (org-agenda-filter-expand-tags filter)))

This line should be removed.

> +     (let ((op (string-to-char x)))
> +       (if expand (setq x (org-agenda-filter-expand-tags (list x) t))
> +         (setq x (list x)))
> +       (setq f1 (org-agenda-filter-make-matcher-tag-exp x op))
> +       (push f1 f))))

Is it useful to bind OP since you use it only once anyway?

> +(defun org-agenda-filter-make-matcher-tag-exp (tags op)

Missing docstring.

> +  (let (f f1) ;f = return expression. f1 = working-area
> +    (dolist (x tags)
> +      (let* ((tag (substring x 1))
> +          (isregexp (and (string-prefix-p "{" tag)
> +                         (string-suffix-p "}" tag)))

See above.

> +          regexp)
> +     (cond
> +      (isregexp
> +       (setq regexp (substring tag 1 -1))
> +       (setq f1 (list 'string-match regexp '(apply 'concat  tags))))

Spurious space.

> +      (t
> +       (setq f1 (list 'member (downcase tag) 'tags))))
> +     (when (equal op ?-)

`eq'

> +    ; any of the expressions can match if op = +
> +    ; all must match if the operator is -. All o

You need two semi-colons here.

> +    (if (equal op ?-)

`eq'

> +(defun org-agenda-filter-apply (filter type &optional expand)
>    "Set FILTER as the new agenda filter and apply it."

See above.

> Subject: [PATCH 3/3] org: Nesting grouptags
>
> * lisp/org.el (org-tags-expand): Nesting grouptags. Allowing subtags
>   to be defined as groups themselves.
>
>   : #+TAGS: [ Group : SubOne(1) SubTwo ]
>   : #+TAGS: [ SubOne : SubOne1 SubOne2 ]
>   : #+TAGS: [ SubTwo : SubTwo1 SubTwo2 ]
>
>   Should be seen as a tree of tags:
>   - Group
>     - SubOne
>       - SubOne1
>       - SubOne2
>     - SubTwo
>       - SubTwo1
>       - SubTwo2
>
>   Searching for "Group" should return all tags defined above.
> ---
>  lisp/org.el | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 6bb8edf..5c80238 100755
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -14543,7 +14543,7 @@ See also `org-scan-tags'.
>                         matcher)))
>      (cons match0 matcher)))
>  
> -(defun org-tags-expand (match &optional single-as-list downcased)
> +(defun org-tags-expand (match &optional single-as-list downcased 
> tags-already-expanded)
>    "Expand group tags in MATCH.

See above.

> +         (when (and (not (get-text-property 0 'grouptag (match-string 2 
> return-match)))
> +                    (not (member tag work-already-expanded)))

Nitpick:

  (unless (or (get-text-property ...)
              (member tag ...)))

> +           (setq work-already-expanded (append (list tag) 
> work-already-expanded))

  (push tag work-already-expanded)

> +           ; Recursively expand each tag in the group, if the tag hasn't
> +           ; already been expanded

See above. Missing full stop, too.

> +                 ;; Redo the regexp-match because the recursive calls seems 
> to mess it up...

You can also use `save-match-data' and restore it later.

Thanks for your work.


Regards,