emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] Refactor org-set-tags arguments for clarity
@ 2017-07-12  2:46 Kaushal Modi
  2017-07-13  7:54 ` Nicolas Goaziou
  0 siblings, 1 reply; 8+ messages in thread
From: Kaushal Modi @ 2017-07-12  2:46 UTC (permalink / raw)
  To: emacs-org list


[-- Attachment #1.1: Type: text/plain, Size: 1085 bytes --]

Hello,

I recently started looking at the org-set-tags function in org.el, but then
quickly got confused with the doc-string.

  "Set the tags for the current headline.
With prefix ARG, realign all tags in headings in the current buffer.
When JUST-ALIGN is non-nil, only align tags."

The purpose of ARG and JUST-ALIGN seems to be the exact same from the
doc-string. On reading the code, I realized that actually ARG should have
been called JUST-ALIGN and the JUST-ALIGN should have been called
ALIGN-ONLY-CURRENT.

The attached patch contains the updated doc-string, refactoring of the
argument names, and renaming of the argument symbol to :align-only-current
from 'align and 'ignore-column in org-set-tag calls. I have left most of
the org-set-tags calls untouched where the argument values are simply t
instead of descriptive 'align or 'ignore-column.

As the patch introduces no functional changes, I have based it off maint.
"make test" is still passing with these changes.

Can you please review the patch and let me know if it's good for committing?

Thanks.
-- 

Kaushal Modi

[-- Attachment #1.2: Type: text/html, Size: 1504 bytes --]

[-- Attachment #2: 0001-Clarify-the-purpose-of-org-set-tags-arguments.patch --]
[-- Type: application/octet-stream, Size: 17279 bytes --]

From f4837c9231e36556a030b4e02dd92a8ac3522029 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Tue, 11 Jul 2017 22:40:38 -0400
Subject: [PATCH] Clarify the purpose of org-set-tags arguments

* lisp/org.el (org-set-tags-command, org-set-tags): Improve argument
  names and doc-string. Auto-indent the functions.

* lisp/org-mouse.el (org-mouse-tag-menu): Remove redundant argument in
  `org-set-tags' call.

* lisp/org.el (org-promote, org-demote, org-priority)
  (org-set-tags-to, org-entry-put):
* lisp/org-mobile.el (org-mobile-edit):
* lisp/org-capture.el (org-capture-fill-template): Use a more accurate
  argument symbol to match the above `org-set-tags' refactoring.

* lisp/org.el (org-fix-tags-on-the-fly): Typo fix in docstring.
---
 lisp/org-capture.el |   2 +-
 lisp/org-mobile.el  |   2 +-
 lisp/org-mouse.el   |   2 +-
 lisp/org.el         | 258 ++++++++++++++++++++++++++++------------------------
 4 files changed, 140 insertions(+), 124 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 1175cb4084..41d9e40d6d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1728,7 +1728,7 @@ The template may still contain \"%?\" for cursor positioning."
 			 (unless (eq (char-after) ?:) (insert ":"))
 			 (and (org-at-heading-p)
 			      (let ((org-ignore-region t))
-				(org-set-tags nil 'align))))))
+				(org-set-tags nil :align-only-current))))))
 		    ((or "C" "L")
 		     (let ((insert-fun (if (equal key "C") #'insert
 					 (lambda (s) (org-insert-link 0 s)))))
diff --git a/lisp/org-mobile.el b/lisp/org-mobile.el
index 12e6c84b3c..065137c53d 100644
--- a/lisp/org-mobile.el
+++ b/lisp/org-mobile.el
@@ -1031,7 +1031,7 @@ be returned that indicates what went wrong."
 	      (goto-char (match-beginning 4))
 	      (insert new)
 	      (delete-region (point) (+ (point) (length current)))
-	      (org-set-tags nil 'align))
+	      (org-set-tags nil :align-only-current))
 	     (t (error "Heading changed in MobileOrg and on the computer")))))))
 
      ((eq what 'addheading)
diff --git a/lisp/org-mouse.el b/lisp/org-mouse.el
index d6a472787e..d662f87673 100644
--- a/lisp/org-mouse.el
+++ b/lisp/org-mouse.el
@@ -435,7 +435,7 @@ SCHEDULED: or DEADLINE: or ANYTHINGLIKETHIS:"
       ))
    '("--"
      ["Align Tags Here" (org-set-tags nil t) t]
-     ["Align Tags in Buffer" (org-set-tags t t) t]
+     ["Align Tags in Buffer" (org-set-tags t) t]
      ["Set Tags ..." (org-set-tags) t])))
 
 (defun org-mouse-set-tags (tags)
diff --git a/lisp/org.el b/lisp/org.el
index eaa85dd853..b2abccf0b6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -8291,7 +8291,7 @@ even level numbers will become the next higher odd number."
        (user-error "Cannot promote to level 0.  UNDO to recover if necessary"))
       (t (replace-match up-head nil t)))
      (unless (= level 1)
-       (when org-auto-align-tags (org-set-tags nil 'ignore-column))
+       (when org-auto-align-tags (org-set-tags nil :align-only-current))
        (when org-adapt-indentation (org-fixup-indentation (- diff))))
      (run-hooks 'org-after-promote-entry-hook))))
 
@@ -8305,7 +8305,7 @@ even level numbers will become the next higher odd number."
 	  (down-head (concat (make-string (org-get-valid-level level 1) ?*) " "))
 	  (diff (abs (- level (length down-head) -1))))
      (replace-match down-head nil t)
-     (when org-auto-align-tags (org-set-tags nil 'ignore-column))
+     (when org-auto-align-tags (org-set-tags nil :align-only-current))
      (when org-adapt-indentation (org-fixup-indentation diff))
      (run-hooks 'org-after-demote-entry-hook))))
 
@@ -14199,7 +14199,7 @@ ACTION can be `set', `up', `down', or a character."
 		  (insert " [#" news "]"))
 	      (goto-char (match-beginning 3))
 	      (insert "[#" news "] "))))
-	(org-set-tags nil 'align))
+	(org-set-tags nil :align-only-current))
       (if remove
 	  (message "Priority removed")
 	(message "Priority of current item set to %s" news)))))
@@ -14955,16 +14955,25 @@ Assume point is on a headline."
 	;; before tags.
 	(when (< pos (point)) (goto-char pos))))))
 
-(defun org-set-tags-command (&optional arg just-align)
-  "Call the set-tags command for the current entry."
+(defun org-set-tags-command (&optional just-align align-only-current)
+  "Set the tags for the current headline.
+With prefix JUST-ALIGN, only re-align all tags in headings in the
+current buffer.  If JUST-ALIGN is nil and ALIGN-ONLY-CURRENT is
+non-nil, and if the point is on a headline, only align tags in
+that heading.
+
+This command is similar to `org-set-tags', but does not need the
+point to be on an heading when called. "
   (interactive "P")
-  (if (or (org-at-heading-p) (and arg (org-before-first-heading-p)))
-      (org-set-tags arg just-align)
+  (if (or (org-at-heading-p)
+          (and just-align
+               (org-before-first-heading-p)))
+      (org-set-tags just-align align-only-current)
     (save-excursion
       (unless (and (org-region-active-p)
-		   org-loop-over-headlines-in-active-region)
-	(org-back-to-heading t))
-      (org-set-tags arg just-align))))
+                   org-loop-over-headlines-in-active-region)
+        (org-back-to-heading t))
+      (org-set-tags just-align align-only-current))))
 
 (defun org-set-tags-to (data)
   "Set the tags of the current entry to DATA, replacing the current tags.
@@ -14990,10 +14999,10 @@ If DATA is nil or the empty string, any tags will be removed."
 	      (goto-char (match-beginning 5))
 	      (insert data)
 	      (delete-region (point) (point-at-eol))
-	      (org-set-tags nil 'align))
+	      (org-set-tags nil :align-only-current))
 	  (goto-char (point-at-eol))
 	  (insert " " data)
-	  (org-set-tags nil 'align)))
+	  (org-set-tags nil :align-only-current)))
       (beginning-of-line 1)
       (when (looking-at ".*?\\([ \t]+\\)$")
 	(delete-region (match-beginning 1) (match-end 1))))))
@@ -15009,127 +15018,134 @@ If DATA is nil or the empty string, any tags will be removed."
       (message "No headings"))))
 
 (defvar org-indent-indentation-per-level)
-(defun org-set-tags (&optional arg just-align)
+(defun org-set-tags (&optional just-align align-only-current)
   "Set the tags for the current headline.
-With prefix ARG, realign all tags in headings in the current buffer.
-When JUST-ALIGN is non-nil, only align tags."
+With prefix JUST-ALIGN, only re-align all tags in headings in the
+current buffer.  If JUST-ALIGN is nil and ALIGN-ONLY-CURRENT is
+non-nil, and if the point is on a headline, only align tags in
+that heading.
+
+It is assumed that the point is in a heading when this function
+is called."
   (interactive "P")
   (if (and (org-region-active-p) org-loop-over-headlines-in-active-region)
       (let ((cl (if (eq org-loop-over-headlines-in-active-region 'start-level)
                     'region-start-level
-		  'region))
+                  'region))
             org-loop-over-headlines-in-active-region)
         (org-map-entries
-         ;; We don't use ARG and JUST-ALIGN here because these args
-         ;; are not useful when looping over headlines.
+         ;; We don't use JUST-ALIGN and ALIGN-ONLY-CURRENT here
+         ;; because these args are not useful when looping over
+         ;; headlines.
          #'org-set-tags
          org-loop-over-headlines-in-active-region
          cl
-	 '(when (org-invisible-p) (org-end-of-subtree nil t))))
+         '(when (org-invisible-p) (org-end-of-subtree nil t))))
     (let ((org-setting-tags t))
-      (if arg
+      (if just-align
           (save-excursion
             (goto-char (point-min))
             (while (re-search-forward org-outline-regexp-bol nil t)
-	      (org-set-tags nil t)
-	      (end-of-line))
+              (org-set-tags nil :align-only-current)
+              (end-of-line))
             (message "All tags realigned to column %d" org-tags-column))
-	(let* ((current (org-get-tags-string))
-	       (tags
-		(if just-align current
-		  ;; Get a new set of tags from the user.
-		  (save-excursion
-		    (let* ((seen)
-			   (table
-			    (setq
-			     org-last-tags-completion-table
-			     ;; Uniquify tags in alists, yet preserve
-			     ;; structure (i.e., keywords).
-			     (delq nil
-				   (mapcar
-				    (lambda (pair)
-				      (let ((head (car pair)))
-					(cond ((symbolp head) pair)
-					      ((member head seen) nil)
-					      (t (push head seen)
-						 pair))))
-				    (append
-				     (or org-current-tag-alist
-					 (org-get-buffer-tags))
-				     (and
-				      org-complete-tags-always-offer-all-agenda-tags
-				      (org-global-tags-completion-table
-				       (org-agenda-files))))))))
-			   (current-tags (org-split-string current ":"))
-			   (inherited-tags
-			    (nreverse (nthcdr (length current-tags)
-					      (nreverse (org-get-tags-at))))))
-		      (replace-regexp-in-string
-		       "\\([-+&]+\\|,\\)"
-		       ":"
-		       (if (or (eq t org-use-fast-tag-selection)
-			       (and org-use-fast-tag-selection
-				    (delq nil (mapcar #'cdr table))))
-			   (org-fast-tag-selection
-			    current-tags inherited-tags table
-			    (and org-fast-tag-selection-include-todo
-				 org-todo-key-alist))
-			 (let ((org-add-colon-after-tag-completion
-				(< 1 (length table))))
-			   (org-trim
-			    (completing-read
-			     "Tags: "
-			     #'org-tags-completion-function
-			     nil nil current 'org-tags-history))))))))))
-
-	  (when org-tags-sort-function
-	    (setq tags
-		  (mapconcat
-		   #'identity
-		   (sort (org-split-string tags "[^[:alnum:]_@#%]+")
-			 org-tags-sort-function)
-		   ":")))
-
-	  (if (or (string= ":" tags)
-		  (string= "::" tags))
-	      (setq tags ""))
-	  (if (not (org-string-nw-p tags)) (setq tags "")
-	    (unless (string-suffix-p ":" tags) (setq tags (concat tags ":")))
-	    (unless (string-prefix-p ":" tags) (setq tags (concat ":" tags))))
-
-	  ;; Insert new tags at the correct column.
-	  (unless (equal current tags)
-	    (save-excursion
-	      (beginning-of-line)
-	      (let ((case-fold-search nil))
-		(looking-at org-complex-heading-regexp))
-	      ;; Remove current tags, if any.
-	      (when (match-end 5) (replace-match "" nil nil nil 5))
-	      ;; Insert new tags, if any.  Otherwise, remove trailing
-	      ;; white spaces.
-	      (end-of-line)
-	      (if (not (equal tags ""))
-		  ;; When text is being inserted on an invisible
-		  ;; region boundary, it can be inadvertently sucked
-		  ;; into invisibility.
-		  (outline-flag-region (point) (progn (insert " " tags) (point)) nil)
-		(skip-chars-backward " \t")
-		(delete-region (point) (line-end-position)))))
-	  ;; Align tags, if any.  Fix tags column if `org-indent-mode'
-	  ;; is on.
-	  (unless (equal tags "")
-	    (let* ((level (save-excursion
-			    (beginning-of-line)
-			    (skip-chars-forward "\\*")))
-		   (offset (if (bound-and-true-p org-indent-mode)
-			       (* (1- org-indent-indentation-per-level)
-				  (1- level))
-			     0))
-		   (tags-column
-		    (+ org-tags-column
-		       (if (> org-tags-column 0) (- offset) offset))))
-	      (org--align-tags-here tags-column))))
-        (unless just-align (run-hooks 'org-after-tags-change-hook))))))
+        (let* ((current (org-get-tags-string))
+               (tags
+                (if align-only-current
+                    current
+                  ;; Get a new set of tags from the user.
+                  (save-excursion
+                    (let* ((seen)
+                           (table
+                            (setq
+                             org-last-tags-completion-table
+                             ;; Uniquify tags in alists, yet preserve
+                             ;; structure (i.e., keywords).
+                             (delq nil
+                                   (mapcar
+                                    (lambda (pair)
+                                      (let ((head (car pair)))
+                                        (cond ((symbolp head) pair)
+                                              ((member head seen) nil)
+                                              (t (push head seen)
+                                                 pair))))
+                                    (append
+                                     (or org-current-tag-alist
+                                         (org-get-buffer-tags))
+                                     (and
+                                      org-complete-tags-always-offer-all-agenda-tags
+                                      (org-global-tags-completion-table
+                                       (org-agenda-files))))))))
+                           (current-tags (org-split-string current ":"))
+                           (inherited-tags
+                            (nreverse (nthcdr (length current-tags)
+                                              (nreverse (org-get-tags-at))))))
+                      (replace-regexp-in-string
+                       "\\([-+&]+\\|,\\)"
+                       ":"
+                       (if (or (eq t org-use-fast-tag-selection)
+                               (and org-use-fast-tag-selection
+                                    (delq nil (mapcar #'cdr table))))
+                           (org-fast-tag-selection
+                            current-tags inherited-tags table
+                            (and org-fast-tag-selection-include-todo
+                                 org-todo-key-alist))
+                         (let ((org-add-colon-after-tag-completion
+                                (< 1 (length table))))
+                           (org-trim
+                            (completing-read
+                             "Tags: "
+                             #'org-tags-completion-function
+                             nil nil current 'org-tags-history))))))))))
+
+          (when org-tags-sort-function
+            (setq tags
+                  (mapconcat
+                   #'identity
+                   (sort (org-split-string tags "[^[:alnum:]_@#%]+")
+                         org-tags-sort-function)
+                   ":")))
+
+          (if (or (string= ":" tags)
+                  (string= "::" tags))
+              (setq tags ""))
+          (if (not (org-string-nw-p tags)) (setq tags "")
+            (unless (string-suffix-p ":" tags) (setq tags (concat tags ":")))
+            (unless (string-prefix-p ":" tags) (setq tags (concat ":" tags))))
+
+          ;; Insert new tags at the correct column.
+          (unless (equal current tags)
+            (save-excursion
+              (beginning-of-line)
+              (let ((case-fold-search nil))
+                (looking-at org-complex-heading-regexp))
+              ;; Remove current tags, if any.
+              (when (match-end 5) (replace-match "" nil nil nil 5))
+              ;; Insert new tags, if any.  Otherwise, remove trailing
+              ;; white spaces.
+              (end-of-line)
+              (if (not (equal tags ""))
+                  ;; When text is being inserted on an invisible
+                  ;; region boundary, it can be inadvertently sucked
+                  ;; into invisibility.
+                  (outline-flag-region (point) (progn (insert " " tags) (point)) nil)
+                (skip-chars-backward " \t")
+                (delete-region (point) (line-end-position)))))
+          ;; Align tags, if any.  Fix tags column if `org-indent-mode'
+          ;; is on.
+          (unless (equal tags "")
+            (let* ((level (save-excursion
+                            (beginning-of-line)
+                            (skip-chars-forward "\\*")))
+                   (offset (if (bound-and-true-p org-indent-mode)
+                               (* (1- org-indent-indentation-per-level)
+                                  (1- level))
+                             0))
+                   (tags-column
+                    (+ org-tags-column
+                       (if (> org-tags-column 0) (- offset) offset))))
+              (org--align-tags-here tags-column))))
+        (unless align-only-current (run-hooks 'org-after-tags-change-hook))))))
 
 (defun org-change-tag-in-region (beg end tag off)
   "Add or remove TAG for each entry in the region.
@@ -16164,10 +16180,10 @@ decreases scheduled or deadline date by one day."
 	      ((not (member value org-todo-keywords-1))
 	       (user-error "\"%s\" is not a valid TODO state" value)))
 	(org-todo value)
-	(org-set-tags nil 'align))
+	(org-set-tags nil :align-only-current))
        ((equal property "PRIORITY")
 	(org-priority (if (org-string-nw-p value) (string-to-char value) ?\s))
-	(org-set-tags nil 'align))
+	(org-set-tags nil :align-only-current))
        ((equal property "SCHEDULED")
 	(forward-line)
 	(if (and (looking-at-p org-planning-line-re)
@@ -20254,7 +20270,7 @@ The detailed reaction depends on the user option `org-catch-invisible-edits'."
 
 (defun org-fix-tags-on-the-fly ()
   "Align tags in headline at point.
-Unlike to `org-set-tags', it ignores region and sorting."
+Unlike `org-set-tags', it ignores region and sorting."
   (when (and (eq (char-after (line-beginning-position)) ?*) ;short-circuit
 	     (org-at-heading-p))
     (let ((org-ignore-region t)
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-07-13 12:39 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-12  2:46 [PATCH] Refactor org-set-tags arguments for clarity Kaushal Modi
2017-07-13  7:54 ` Nicolas Goaziou
2017-07-13 10:18   ` Kaushal Modi
2017-07-13 11:58     ` Nicolas Goaziou
2017-07-13 12:21       ` Kaushal Modi
2017-07-13 12:31         ` Nicolas Goaziou
2017-07-13 12:37           ` Kaushal Modi
2017-07-13 12:39             ` Kaushal Modi

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).