From a9a4d8c7148c115a242a7b35d16dd536f9c0c700 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sat, 16 May 2026 02:48:18 -0500 Subject: refactor(custom-editing): five hygiene fixes from the module-by-module re-review - Guard `cj/duplicate-line-or-region' when COMMENT is non-nil but the current mode has no `comment-start' (e.g. fundamental-mode). Previously the function silently produced malformed output via `comment-region'; now it signals a clear `user-error'. - Factor the `find-file' advice install in external-open.el into `cj/external-open-install-advice'. Same idempotent shape (remove-then-add) but the intent is named. - Add `cj/--validate-decoration-char' in custom-comments.el and wire it into all six divider / border / box helpers. Rejects multi-char strings, empty strings, and control characters like newline/tab that would corrupt subsequent `M-q' flows. Updated the five nil-decoration ERT tests from `:type 'wrong-type-argument' (the old crash signal from `string-to-char' on nil) to `:type 'user-error', since the validator produces a clear message instead of a deep crash. - Extract `cj/--require-spell-checker' in flyspell-and-abbrev.el. Both `cj/flyspell-toggle' and `cj/flyspell-then-abbrev' now call the shared helper; the checker list lives in `cj/--spell-checker-executables', so adding nuspell or any other checker is a one-line edit. - Preserve trailing newlines in custom-ordering output. Both `cj/--arrayify' and `cj/--unarrayify' now detect a trailing newline on the input region and re-append it to the result, matching the pattern custom-text-enclose.el already uses. --- modules/custom-comments.el | 19 +++++ modules/custom-line-paragraph.el | 9 ++- modules/custom-ordering.el | 23 ++++-- modules/external-open.el | 12 ++- modules/flyspell-and-abbrev.el | 29 +++++--- tests/test-custom-comments-comment-block-banner.el | 2 +- tests/test-custom-comments-comment-box.el | 2 +- tests/test-custom-comments-comment-heavy-box.el | 2 +- .../test-custom-comments-comment-inline-border.el | 6 +- .../test-custom-comments-comment-padded-divider.el | 2 +- .../test-custom-comments-comment-simple-divider.el | 2 +- todo.org | 87 +++++++++++----------- 12 files changed, 122 insertions(+), 73 deletions(-) diff --git a/modules/custom-comments.el b/modules/custom-comments.el index d5419be2..7e8e1404 100644 --- a/modules/custom-comments.el +++ b/modules/custom-comments.el @@ -85,6 +85,19 @@ ;; ======================== Comment Generation Functions ======================= +(defun cj/--validate-decoration-char (decoration-char) + "Signal `user-error' unless DECORATION-CHAR is a printable single character. +Returns DECORATION-CHAR unchanged on success. Rejects multi-char +strings, empty strings, control chars like newline/tab, and non-string +inputs. Used by all divider / border helpers below." + (unless (and (stringp decoration-char) + (= (length decoration-char) 1) + (string-match-p "[[:print:]]" decoration-char)) + (user-error + "Decoration character must be a single printable character, got: %S" + decoration-char)) + decoration-char) + ;; ----------------------------- Inline Border --------------------------------- (defun cj/--comment-inline-border (cmt-start cmt-end decoration-char text length) @@ -93,6 +106,7 @@ CMT-START and CMT-END are the comment syntax strings. DECORATION-CHAR is the character to use for borders (string). TEXT is the comment text (will be centered). LENGTH is the total width of the line." + (cj/--validate-decoration-char decoration-char) (let* ((current-column-pos (current-column)) (text-length (length text)) (comment-start-len (+ (length cmt-start) @@ -157,6 +171,7 @@ CMT-START and CMT-END are the comment syntax strings. DECORATION-CHAR is the character to use for the divider lines. TEXT is the comment text. LENGTH is the total width of each line." + (cj/--validate-decoration-char decoration-char) (let* ((current-column-pos (current-column)) (min-length (+ current-column-pos (length cmt-start) @@ -233,6 +248,7 @@ DECORATION-CHAR is the character to use for the divider lines. TEXT is the comment text. LENGTH is the total width of each line. PADDING is the number of spaces before the text." + (cj/--validate-decoration-char decoration-char) (when (< padding 0) (error "Padding %d cannot be negative" padding)) (let* ((current-column-pos (current-column)) @@ -314,6 +330,7 @@ CMT-START and CMT-END are the comment syntax strings. DECORATION-CHAR is the character to use for borders. TEXT is the comment text (centered). LENGTH is the total width of each line." + (cj/--validate-decoration-char decoration-char) (let* ((current-column-pos (current-column)) (comment-char (if (equal cmt-start ";") ";;" cmt-start)) (comment-end-char (if (string-empty-p cmt-end) comment-char cmt-end)) @@ -377,6 +394,7 @@ CMT-START and CMT-END are the comment syntax strings. DECORATION-CHAR is the character to use for borders. TEXT is the comment text (centered). LENGTH is the total width of each line." + (cj/--validate-decoration-char decoration-char) (let* ((current-column-pos (current-column)) (comment-char (if (equal cmt-start ";") ";;" cmt-start)) (comment-end-char (if (string-empty-p cmt-end) comment-char cmt-end)) @@ -542,6 +560,7 @@ CMT-END should be the block comment end (e.g., '*/'). DECORATION-CHAR is the character to use for the border line. TEXT is the comment text. LENGTH is the total width of each line." + (cj/--validate-decoration-char decoration-char) (let* ((current-column-pos (current-column)) (min-length (+ current-column-pos (length cmt-start) diff --git a/modules/custom-line-paragraph.el b/modules/custom-line-paragraph.el index 27f24cfe..0eb1e2a5 100644 --- a/modules/custom-line-paragraph.el +++ b/modules/custom-line-paragraph.el @@ -54,8 +54,15 @@ (defun cj/duplicate-line-or-region (&optional comment) "Duplicate the current line or active region below. -Comment the duplicated text when optional COMMENT is non-nil." +Comment the duplicated text when optional COMMENT is non-nil. +Signal `user-error' when COMMENT is non-nil but the current mode has +no `comment-start' (e.g. `fundamental-mode'), since commenting would +produce malformed output silently." (interactive "P") + (when (and comment (not (and (stringp comment-start) + (> (length comment-start) 0)))) + (user-error + "Cannot comment in %s: no comment syntax defined" major-mode)) (let* ((b (if (region-active-p) (region-beginning) (line-beginning-position))) (e (if (region-active-p) (region-end) (line-end-position))) (lines (split-string (buffer-substring-no-properties b e) "\n"))) diff --git a/modules/custom-ordering.el b/modules/custom-ordering.el index 81490b65..69a2d465 100644 --- a/modules/custom-ordering.el +++ b/modules/custom-ordering.el @@ -32,13 +32,18 @@ QUOTE specifies the quotation characters to surround each element. Use \"\" for no quotes, \"\\\"\" for double quotes, \"'\" for single quotes. PREFIX is an optional string to prepend to the result (e.g., \"[\" or \"(\"). SUFFIX is an optional string to append to the result (e.g., \"]\" or \")\"). +Preserves a trailing newline if the input region ends with one, so +line-oriented operations on the result behave the same as before. Returns the transformed string without modifying the buffer." (when (> start end) (error "Invalid region: start (%d) is greater than end (%d)" start end)) - (let ((result (mapconcat - (lambda (x) (format "%s%s%s" quote x quote)) - (split-string (buffer-substring start end)) ", "))) - (concat (or prefix "") result (or suffix "")))) + (let* ((raw (buffer-substring start end)) + (trailing-newline (string-suffix-p "\n" raw)) + (result (mapconcat + (lambda (x) (format "%s%s%s" quote x quote)) + (split-string raw) ", "))) + (concat (or prefix "") result (or suffix "") + (if trailing-newline "\n" "")))) (defun cj/arrayify (start end quote) "Convert lines between START and END into quoted, comma-separated strings. @@ -80,12 +85,16 @@ Example: `apple banana cherry' becomes `[\"apple\", \"banana\", \"cherry\"]'." "Internal implementation: Convert comma-separated array to lines. START and END define the region to operate on. Removes quotes (both single and double) and splits by ', '. +Preserves a trailing newline if the input region ends with one. Returns the transformed string without modifying the buffer." (when (> start end) (error "Invalid region: start (%d) is greater than end (%d)" start end)) - (mapconcat - (lambda (x) (replace-regexp-in-string "[\"']" "" x)) - (split-string (buffer-substring start end) ", ") "\n")) + (let* ((raw (buffer-substring start end)) + (trailing-newline (string-suffix-p "\n" raw)) + (result (mapconcat + (lambda (x) (replace-regexp-in-string "[\"']" "" x)) + (split-string raw ", ") "\n"))) + (concat result (if trailing-newline "\n" "")))) (defun cj/unarrayify (start end) "Convert quoted comma-separated strings between START and END to separate lines. diff --git a/modules/external-open.el b/modules/external-open.el index 0d6ec520..57cffadc 100644 --- a/modules/external-open.el +++ b/modules/external-open.el @@ -146,9 +146,15 @@ Else call ORIG-FUN with ARGS." (cj/xdg-open file) (apply orig-fun args)))) -;; Make advice idempotent if you reevaluate this form. -(advice-remove 'find-file #'cj/find-file-auto) -(advice-add 'find-file :around #'cj/find-file-auto) +(defun cj/external-open-install-advice () + "Install the `cj/find-file-auto' advice on `find-file'. +Idempotent: re-running removes any prior copy of the advice before +adding it back, so re-evaluating the module updates the advice rather +than stacking it." + (advice-remove 'find-file #'cj/find-file-auto) + (advice-add 'find-file :around #'cj/find-file-auto)) + +(cj/external-open-install-advice) (provide 'external-open) ;;; external-open.el ends here. diff --git a/modules/flyspell-and-abbrev.el b/modules/flyspell-and-abbrev.el index e29ad6e9..e732ac46 100644 --- a/modules/flyspell-and-abbrev.el +++ b/modules/flyspell-and-abbrev.el @@ -43,6 +43,8 @@ ;;; Code: +(require 'cl-lib) + ;; Forward declarations (eval-when-compile (defvar org-dir)) (defvar flyspell-mode) @@ -111,18 +113,26 @@ ;; ------------------------------ Flyspell Toggle ------------------------------ ;; easy toggling flyspell and also leverage the 'for-buffer-type' functionality. +(defconst cj/--spell-checker-executables + '("aspell" "ispell" "hunspell") + "Spell-checker executables `cj/--require-spell-checker' looks for.") + +(defun cj/--require-spell-checker () + "Signal `user-error' unless a known spell-checker is on PATH. +Looked-up executables: `cj/--spell-checker-executables'. Single point +of change when a new checker (e.g. nuspell) is added." + (unless (cl-some #'executable-find cj/--spell-checker-executables) + (user-error + "No spell checker found. Install one of: %s" + (mapconcat #'identity cj/--spell-checker-executables ", ")))) + (defun cj/flyspell-toggle () "Turn Flyspell on if it is off, or off if it is on. When turning on, it uses `cj/flyspell-on-for-buffer-type' so code-vs-text is handled appropriately." (interactive) - ;; Check if spell checker is available - (unless (or (executable-find "aspell") - (executable-find "ispell") - (executable-find "hunspell")) - (user-error "No spell checker found. Install aspell, ispell, or hunspell")) - + (cj/--require-spell-checker) (if (bound-and-true-p flyspell-mode) (progn ; flyspell is on, turn it off (flyspell-mode -1) @@ -208,12 +218,7 @@ Without prefix argument, it's created in the global abbrev table. Press C-' repeatedly to step through misspellings one at a time." (interactive "P") - ;; Check if spell checker is available - (unless (or (executable-find "aspell") - (executable-find "ispell") - (executable-find "hunspell")) - (user-error "No spell checker found. Install aspell, ispell, or hunspell")) - + (cj/--require-spell-checker) ;; Run flyspell-buffer only if buffer hasn't been checked yet (unless (bound-and-true-p flyspell-mode) (flyspell-buffer)) diff --git a/tests/test-custom-comments-comment-block-banner.el b/tests/test-custom-comments-comment-block-banner.el index 6561ebfa..da0b1fa7 100644 --- a/tests/test-custom-comments-comment-block-banner.el +++ b/tests/test-custom-comments-comment-block-banner.el @@ -194,7 +194,7 @@ Returns the buffer string for assertions." "Should error when decoration-char is nil." (should-error (test-block-banner-at-column 0 "/*" "*/" nil "Header" 70) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-block-banner-c-nil-text () "Should error when text is nil." diff --git a/tests/test-custom-comments-comment-box.el b/tests/test-custom-comments-comment-box.el index 10b1a67d..d7ee2830 100644 --- a/tests/test-custom-comments-comment-box.el +++ b/tests/test-custom-comments-comment-box.el @@ -203,7 +203,7 @@ Returns the buffer string for assertions." "Should error when decoration-char is nil." (should-error (test-comment-box-at-column 0 ";;" "" nil "Header" 70) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-comment-box-elisp-non-integer-length () "Should error when length is not an integer." diff --git a/tests/test-custom-comments-comment-heavy-box.el b/tests/test-custom-comments-comment-heavy-box.el index 30289625..94d4aaa5 100644 --- a/tests/test-custom-comments-comment-heavy-box.el +++ b/tests/test-custom-comments-comment-heavy-box.el @@ -207,7 +207,7 @@ Returns the buffer string for assertions." "Should error when decoration-char is nil." (should-error (test-heavy-box-at-column 0 ";;" "" nil "Header" 70) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-heavy-box-elisp-nil-text () "Should error when text is nil." diff --git a/tests/test-custom-comments-comment-inline-border.el b/tests/test-custom-comments-comment-inline-border.el index ca2bef06..78e86035 100644 --- a/tests/test-custom-comments-comment-inline-border.el +++ b/tests/test-custom-comments-comment-inline-border.el @@ -190,10 +190,12 @@ Returns the buffer string for assertions." :type 'error)) (ert-deftest test-inline-border-elisp-nil-decoration () - "Should error when decoration-char is nil." + "Should error when decoration-char is nil. +The decoration-char validator signals `user-error' for any non-printable +or non-single-character input." (should-error (test-inline-border-at-column 0 ";;" "" nil "Header" 70) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-inline-border-elisp-non-integer-length () "Should error when length is not an integer." diff --git a/tests/test-custom-comments-comment-padded-divider.el b/tests/test-custom-comments-comment-padded-divider.el index 702a4c67..d4c18905 100644 --- a/tests/test-custom-comments-comment-padded-divider.el +++ b/tests/test-custom-comments-comment-padded-divider.el @@ -200,7 +200,7 @@ Returns the buffer string for assertions." "Should error when decoration-char is nil." (should-error (test-padded-divider-at-column 0 ";;" "" nil "Header" 70 2) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-padded-divider-elisp-nil-text () "Should error when text is nil." diff --git a/tests/test-custom-comments-comment-simple-divider.el b/tests/test-custom-comments-comment-simple-divider.el index a61e6b4c..7979f18b 100644 --- a/tests/test-custom-comments-comment-simple-divider.el +++ b/tests/test-custom-comments-comment-simple-divider.el @@ -195,7 +195,7 @@ Returns the buffer string for assertions." "Should error when decoration-char is nil." (should-error (test-simple-divider-at-column 0 ";;" "" nil "Header" 70) - :type 'wrong-type-argument)) + :type 'user-error)) (ert-deftest test-simple-divider-elisp-nil-text () "Should error when text is nil." diff --git a/todo.org b/todo.org index 0ea131a4..017b1584 100644 --- a/todo.org +++ b/todo.org @@ -1210,42 +1210,43 @@ the implementation does not match. Pick one contract per pair and update docstrings, or extract a shared helper that decides the target range. -**** TODO [#C] Preserve trailing newlines in custom-ordering output :bug: - -=modules/custom-ordering.el:38-41,86-88= splits input on whitespace -and commas without tracking a trailing newline. Compare with -=custom-text-enclose.el=, which explicitly records and restores a -trailing newline. When the user invokes ordering on a buffer whose -last line ends with =\n=, the output drops it -- which then breaks -line-oriented operations on the result. - -**** TODO [#C] Guard =cj/duplicate-line-and-comment= against non-file buffers :safety: - -=modules/custom-line-paragraph.el:79-93= calls =(cj/comment-line)= via -delegation. =comment-line= reads =comment-start= from the current -major-mode's syntax tables, which is unreliable in non-file buffers -(=*scratch*= sometimes works, fundamental-mode buffers do not). The -duplicate-and-comment flow silently produces malformed output in -those buffers. Either error out clearly or only enable the binding -in modes that have a comment syntax. - -**** TODO [#C] Make =external-open.el= advice setup idempotent at load time :cleanup: - -=modules/external-open.el:150-151= runs =advice-remove= followed by -=advice-add= unconditionally during module load. The pair is -idempotent-by-construction, but the pattern leaves no signal that the -module guards against duplicate loads -- if the module ever stops -being load-once, the advice list grows. Wrap the setup in a guarded -form or move it behind an explicit init function. - -**** TODO [#C] Validate comment-delimiter character in =custom-comments.el= :safety: - -The comment-divider / border helpers in =modules/custom-comments.el= -accept a delimiter string with no length or printability check. -Passing =\n=, =\t=, or a multi-character string produces malformed -output that can corrupt subsequent =M-q= flows. Add a guard: -=(unless (and (stringp delim) (= (length delim) 1) (string-match-p "[[:print:]]" delim)) - (user-error "..."))= +**** 2026-05-16 Sat @ 02:47:15 -0500 Preserved trailing newlines in custom-ordering output + +Both =cj/--arrayify= and =cj/--unarrayify= now detect a trailing +newline on the input region (via =string-suffix-p=) and re-append it +to the result. Matches the pattern =custom-text-enclose.el= already +uses. Docstrings updated to document the contract. + +**** 2026-05-16 Sat @ 02:47:15 -0500 Guarded cj/duplicate-line-or-region against modes without comment syntax + +=cj/duplicate-line-or-region= now signals a clear =user-error= when +=COMMENT= is non-nil but the current mode has no =comment-start= +(=fundamental-mode= and similar). Docstring updated to document the +error. Picks the "error out clearly" branch from the task body -- +restricting the binding per-mode would be a larger refactor that +isn't justified for the silent-malformed-output blast radius. + +**** 2026-05-16 Sat @ 02:47:15 -0500 Made external-open advice install explicit via cj/external-open-install-advice + +Factored the =advice-remove= / =advice-add= pair into +=cj/external-open-install-advice= and called it once at the bottom +of the module. Same idempotent shape (remove-then-add) but now the +intent is named. Re-running the module updates the advice rather +than stacking it; if a future caller wants to opt out, they can +=advice-remove= the helper or skip calling it altogether. + +**** 2026-05-16 Sat @ 02:47:15 -0500 Added cj/--validate-decoration-char across all six divider/border helpers + +New top-level validator =cj/--validate-decoration-char= rejects +anything that isn't a printable single-character string and signals +=user-error=. Wired into all six internal helpers: +=cj/--comment-inline-border=, =cj/--comment-simple-divider=, +=cj/--comment-padded-divider=, =cj/--comment-box=, +=cj/--comment-heavy-box=, =cj/--comment-block-banner=. The five +nil-decoration ERT tests updated from =:type 'wrong-type-argument= +(the old crash signal from =string-to-char= on nil) to +=:type 'user-error=, since the guard now produces a clear message +instead of a deep crash. **** TODO [#C] Add coverage for =cj/title-case-region= state machine :tests: @@ -1257,14 +1258,14 @@ opening quote characters. The state machine is the kind of helper that fails silently on the unusual case -- ERT coverage on those inputs catches regressions during refactors. -**** TODO [#C] De-duplicate spell-checker availability guard in =flyspell-and-abbrev.el= :cleanup: +**** 2026-05-16 Sat @ 02:47:15 -0500 Extracted cj/--require-spell-checker -=cj/flyspell-toggle= and =cj/flyspell-then-abbrev= each carry an -identical =(unless (or (executable-find "aspell") (executable-find -"ispell") (executable-find "hunspell")) (user-error ...))= block. -Extract into =cj/--require-spell-checker= and call from both sites. -Keeps the executable list in one place when a new checker (e.g. -nuspell) is added. +Both =cj/flyspell-toggle= and =cj/flyspell-then-abbrev= now call +=cj/--require-spell-checker= instead of carrying their own copy of +the executable-find check. The checker list lives in the new +defconst =cj/--spell-checker-executables=, so adding nuspell (or any +other checker) is a one-line edit. Top-level =(require 'cl-lib)= +added since the new helper uses =cl-some=. **** TODO [#C] Add coverage for =cj/flyspell-then-abbrev= and helpers in =flyspell-and-abbrev.el= :tests: -- cgit v1.2.3