refactor: Address code review feedback

- Add tp--ensure-props helper to reduce code duplication
- Add nil check for group's first layer in tp--resolve-props
- Improve comments in tp--parse-args to explain the unwrapping logic

Co-authored-by: Kinneyzhang <38454496+Kinneyzhang@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot] 2025-12-20 07:20:45 +00:00
parent 8e9b480d4c
commit f775d761bf

48
tp.el
View File

@ -275,10 +275,15 @@ by `define-tp' or `define-tp-group', which will be resolved to its properties."
(setq object nil (setq object nil
props props-or-val))) props props-or-val)))
(t (error "Invalid first argument: %S" start-or-string))) (t (error "Invalid first argument: %S" start-or-string)))
;; Resolve layer/group name to properties if props is a symbol ;; Resolve layer/group name to properties if props is a symbol.
;; This allows passing layer names like 'my-layer instead of property lists.
(when (symbolp props) (when (symbolp props)
(setq props (or (tp--resolve-props props) props))) (setq props (or (tp--resolve-props props) props)))
;; Handle properties as a list (only if props is a list and its first element is also a list) ;; Unwrap double-wrapped properties: when called as (tp-set 1 6 '(face bold)),
;; props is already the plist. But when called internally or from certain
;; contexts, props might be wrapped in an extra list like '((face bold)).
;; We detect this by checking if props is a list whose first element is also
;; a list (not just a symbol like 'face).
(when (and (listp props) (listp (car-safe props))) (when (and (listp props) (listp (car-safe props)))
(setq props (car props))) (setq props (car props)))
(list object start finish props))) (list object start finish props)))
@ -1062,8 +1067,7 @@ OBJECT is a buffer or string; nil means current buffer.
Returns: Returns:
- For strings: the modified string - For strings: the modified string
- For buffers: list of (START . END) pairs for all matches." - For buffers: list of (START . END) pairs for all matches."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--match-apply pattern (tp--ensure-props plist) #'tp-set object))
(tp--match-apply pattern props #'tp-set object)))
(defun tp-match-reset (pattern plist &optional object) (defun tp-match-reset (pattern plist &optional object)
"Reset (completely replace) properties on all occurrences of PATTERN. "Reset (completely replace) properties on all occurrences of PATTERN.
@ -1077,11 +1081,10 @@ or `define-tp-group'.
OBJECT is a buffer or string; nil means current buffer. OBJECT is a buffer or string; nil means current buffer.
Unlike `tp-match-set', this completely replaces all existing properties." Unlike `tp-match-set', this completely replaces all existing properties."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--match-apply pattern (tp--ensure-props plist)
(tp--match-apply pattern props
(lambda (start end props obj) (lambda (start end props obj)
(set-text-properties start end props obj)) (set-text-properties start end props obj))
object))) object))
(defun tp-match-add (pattern plist &optional object) (defun tp-match-add (pattern plist &optional object)
"Add/update properties on all occurrences of PATTERN. "Add/update properties on all occurrences of PATTERN.
@ -1095,8 +1098,7 @@ or `define-tp-group'.
OBJECT is a buffer or string; nil means current buffer. OBJECT is a buffer or string; nil means current buffer.
Unlike `tp-match-set', this deeply merges nested properties." Unlike `tp-match-set', this deeply merges nested properties."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--match-apply pattern (tp--ensure-props plist) #'tp--deep-merge-apply object))
(tp--match-apply pattern props #'tp--deep-merge-apply object)))
(defun tp-regexp-set (pattern plist &optional object) (defun tp-regexp-set (pattern plist &optional object)
"Set properties on all matches of PATTERN (regexp). "Set properties on all matches of PATTERN (regexp).
@ -1113,8 +1115,7 @@ OBJECT is a buffer or string; nil means current buffer.
Returns: Returns:
- For strings: the modified string - For strings: the modified string
- For buffers: list of (START . END) pairs for all matches." - For buffers: list of (START . END) pairs for all matches."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--regexp-apply pattern (tp--ensure-props plist) #'tp-set object))
(tp--regexp-apply pattern props #'tp-set object)))
(defun tp-regexp-reset (pattern plist &optional object) (defun tp-regexp-reset (pattern plist &optional object)
"Reset (completely replace) properties on all regexp matches of PATTERN. "Reset (completely replace) properties on all regexp matches of PATTERN.
@ -1128,11 +1129,10 @@ or `define-tp-group'.
OBJECT is a buffer or string; nil means current buffer. OBJECT is a buffer or string; nil means current buffer.
Unlike `tp-regexp-set', this completely replaces all existing properties." Unlike `tp-regexp-set', this completely replaces all existing properties."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--regexp-apply pattern (tp--ensure-props plist)
(tp--regexp-apply pattern props
(lambda (start end props obj) (lambda (start end props obj)
(set-text-properties start end props obj)) (set-text-properties start end props obj))
object))) object))
(defun tp-regexp-add (pattern plist &optional object) (defun tp-regexp-add (pattern plist &optional object)
"Add/update properties on all regexp matches of PATTERN. "Add/update properties on all regexp matches of PATTERN.
@ -1146,8 +1146,7 @@ or `define-tp-group'.
OBJECT is a buffer or string; nil means current buffer. OBJECT is a buffer or string; nil means current buffer.
Unlike `tp-regexp-set', this deeply merges nested properties." Unlike `tp-regexp-set', this deeply merges nested properties."
(let ((props (if (symbolp plist) (or (tp--resolve-props plist) plist) plist))) (tp--regexp-apply pattern (tp--ensure-props plist) #'tp--deep-merge-apply object))
(tp--regexp-apply pattern props #'tp--deep-merge-apply object)))
;;; Search functions ;;; Search functions
@ -1963,6 +1962,9 @@ If PROPS is a symbol:
- First checks `tp-layer-alist' and returns the layer properties - First checks `tp-layer-alist' and returns the layer properties
- Then checks `tp-layer-groups' and returns the first layer's properties - Then checks `tp-layer-groups' and returns the first layer's properties
Returns nil if PROPS is a symbol but no matching layer/group is found,
or if the group's first layer doesn't exist in `tp-layer-alist'.
Unlike `tp-layer-props', this does NOT add the `tp-name' property, Unlike `tp-layer-props', this does NOT add the `tp-name' property,
making it suitable for use with basic property-setting APIs like making it suitable for use with basic property-setting APIs like
`tp-set', `tp-add', `tp-match-set', etc." `tp-set', `tp-add', `tp-match-set', etc."
@ -1978,12 +1980,22 @@ making it suitable for use with basic property-setting APIs like
;; Check group (use first layer's properties) ;; Check group (use first layer's properties)
((assoc props tp-layer-groups) ((assoc props tp-layer-groups)
(when-let* ((layers (cdr (assoc props tp-layer-groups))) (when-let* ((layers (cdr (assoc props tp-layer-groups)))
(first-layer (car layers))) (first-layer (car layers))
(cdr (assoc first-layer tp-layer-alist)))) ;; Ensure the first layer exists in tp-layer-alist
(layer-entry (assoc first-layer tp-layer-alist)))
(cdr layer-entry)))
;; Not found - return nil (let caller decide how to handle) ;; Not found - return nil (let caller decide how to handle)
(t nil))) (t nil)))
(t nil))) (t nil)))
(defun tp--ensure-props (plist)
"Ensure PLIST is a property list, resolving layer names if needed.
If PLIST is a symbol, resolve it via `tp--resolve-props'.
If resolution fails, return PLIST unchanged (for backward compatibility)."
(if (symbolp plist)
(or (tp--resolve-props plist) plist)
plist))
(defun tp-layer-reset () (defun tp-layer-reset ()
"Reset all layer definitions. "Reset all layer definitions.
Clears both `tp-layer-alist' and `tp-layer-groups'. Clears both `tp-layer-alist' and `tp-layer-groups'.