From 256027866f9ff2cbc3b6155950f21a92b54ea307 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Mon, 29 Jun 2026 04:19:31 -0400 Subject: refactor: prefix two collision-prone helpers, document naming audit Two owned helpers carried unprefixed generic names that risk colliding in the single Emacs namespace: car-member (local-repository.el) and unpropertize-kill-ring (system-defaults.el). I renamed them to localrepo--car-member and cj/--unpropertize-kill-ring and updated their callers and tests. Both are non-interactive and contained, so no alias was needed. docs/design/naming-audit.org records the rest of the scan: the allowlist of deliberate module prefixes, the foreign forward-declarations that aren't owned definitions, and a deferred list (keybound commands, the with-timer macro, the ui-theme defcustoms, the user-constants paths) that each want a focused pass rather than an unattended rename. --- docs/design/naming-audit.org | 91 ++++++++++++++++++++++++++++++ modules/local-repository.el | 6 +- modules/system-defaults.el | 4 +- tests/test-local-repository--car-member.el | 40 ++++++------- tests/test-system-defaults-functions.el | 8 +-- tests/test-system-defaults.el | 2 +- 6 files changed, 121 insertions(+), 30 deletions(-) create mode 100644 docs/design/naming-audit.org diff --git a/docs/design/naming-audit.org b/docs/design/naming-audit.org new file mode 100644 index 000000000..f053ccb25 --- /dev/null +++ b/docs/design/naming-audit.org @@ -0,0 +1,91 @@ +#+TITLE: Naming Audit — Public/Private Boundaries in Owned Elisp +#+AUTHOR: Craig Jennings +#+DATE: 2026-06-29 + +* Purpose + +A one-time audit of symbol-naming boundaries across config-owned Elisp, plus +the standing allowlist future reviews cite instead of re-arguing. The +convention itself lives in =.claude/rules/elisp.md=; this file records what the +scan found and how each finding was classified. + +* Convention (summary) + +- User-facing commands and shared helpers: =cj/name=. +- Private helpers inside cj-owned modules: =cj/--name=. +- Package-like standalone modules: =package-name= for public API, + =package--name= for internals (e.g. =calendar-sync--=, =mouse-trap--=, + =localrepo-=). +- No new unprefixed =defun=/=defvar=/=defcustom=/=defconst= in owned modules. +- Tested private helpers may stay private; the test references them directly. + +* Scan method + +#+begin_src sh +rg -n '^\((defun|defvar|defcustom|defconst|defgroup|defmacro) [^ )/]+' modules custom +#+end_src + +The raw scan over-reports. Two large classes are not findings: + +- *Foreign forward-declarations.* A bare =(defvar foreign-var)= with no value + declares another package's special variable to quiet the byte-compiler. It + defines nothing owned. The bulk of the raw hits are these (=org-*=, =emms-*=, + =lsp-*=, =eat-*=, =mu4e-*=, and so on). +- *Vendored third-party files* under =custom/= (=elpa-mirror.el= =elpamr-=, + =eplot.el= =eplot-=). These keep their upstream prefixes by policy. + +* Findings + +** Renamed (clear low-risk, done 2026-06-29) + +| Old | New | Why | +|------------------------------------------+-------------------------------+-------------------------------------------| +| =car-member= (local-repository.el) | =localrepo--car-member= | Unprefixed, generic name with real | +| | | collision risk; pure non-interactive | +| | | helper used only within its module + | +| | | test. | +|------------------------------------------+-------------------------------+-------------------------------------------| +| =unpropertize-kill-ring= | =cj/--unpropertize-kill-ring= | Unprefixed; non-interactive | +| (system-defaults.el) | | =kill-emacs-hook= function, contained to | +| | | its module + tests. | +|------------------------------------------+-------------------------------+-------------------------------------------| + +** Acceptable package prefixes (allowlist — no change) + +These are deliberate, consistent module prefixes, not unprefixed leaks: + +- =env-= / =env--= — host-environment.el. +- =localrepo-= — local-repository.el (public API + defcustoms). +- =mouse-trap-= / =mouse-trap--= — mousetrap-mode.el. +- =show-kill-= — show-kill-ring.el. +- =my-eww-= / =my-eww--= — eww-config.el. +- =signel-= / =signel--= — signal-config.el. + +** Deferred — needs a focused, approved pass (not unattended) + +Each carries a risk that argues for Craig's eyes rather than a no-approvals +rename: + +- *Keybound / interactive commands*: =toggle-window-split= (bound =M-S-t=), + =ensure-macros-file=, =empty-kill-ring=. A rename touches muscle-memory keys + and =M-x= history; do it with =defalias= shims and a live check. +- *Cross-module macro*: =with-timer= (config-utilities.el, used in wrap-up.el + and an architecture test). A macro rename forces recompilation of every + caller; verify the whole graph. +- *defcustoms*: =theme-file=, =fallback-theme-name= (ui-theme.el). Renaming a + =defcustom= orphans any persisted Customize value; use + =define-obsolete-variable-alias=. (No custom-file currently sets them, so the + risk is latent, not active.) +- *High-blast user constants*: the unprefixed =*-dir= / =*-file= constants in + user-constants.el (=org-dir=, =projects-dir=, =books-dir=, and so on) are + referenced across the whole config. They want their own dedicated rename + task, not a drive-by. + +* Acceptance + +- The scan resolves to: two renames done, a documented allowlist of acceptable + prefixes, and a deferred list with rationale. No unexplained unprefixed owned + symbols remain. +- The two renames are covered by their existing module tests (updated in place). +- Future module reviews cite this file and =elisp.md= rather than re-deriving + the boundary. diff --git a/modules/local-repository.el b/modules/local-repository.el index a9df09d38..e3c7a227a 100644 --- a/modules/local-repository.el +++ b/modules/local-repository.el @@ -24,7 +24,7 @@ ;; ------------------------------ Utility Function ----------------------------- -(defun car-member (value list) +(defun localrepo--car-member (value list) "Check if VALUE exists as the car of any cons cell in LIST." (member value (mapcar #'car list))) @@ -65,11 +65,11 @@ keep them in source control." (defun localrepo-initialize () "Add the repository to the package archives, then gives it a high priority." - (unless (car-member localrepo-repository-id package-archives) + (unless (localrepo--car-member localrepo-repository-id package-archives) (add-to-list 'package-archives (cons localrepo-repository-id localrepo-repository-location))) - (unless (car-member localrepo-repository-id package-archive-priorities) + (unless (localrepo--car-member localrepo-repository-id package-archive-priorities) (add-to-list 'package-archive-priorities (cons localrepo-repository-id localrepo-repository-priority)))) diff --git a/modules/system-defaults.el b/modules/system-defaults.el index 7230103cc..c63ca0093 100644 --- a/modules/system-defaults.el +++ b/modules/system-defaults.el @@ -266,10 +266,10 @@ appears only once per session." ;; ------------------ Unpropertize Kill Ring For Performance ----------------- -(defun unpropertize-kill-ring () +(defun cj/--unpropertize-kill-ring () (setq kill-ring (mapcar 'substring-no-properties kill-ring))) -(add-hook 'kill-emacs-hook 'unpropertize-kill-ring) +(add-hook 'kill-emacs-hook 'cj/--unpropertize-kill-ring) ;; ------------------------------- GNU 'ls' On BSD ------------------------------- diff --git a/tests/test-local-repository--car-member.el b/tests/test-local-repository--car-member.el index 8b8c9a7db..30ae58c6b 100644 --- a/tests/test-local-repository--car-member.el +++ b/tests/test-local-repository--car-member.el @@ -1,7 +1,7 @@ -;;; test-local-repository--car-member.el --- Tests for car-member -*- lexical-binding: t -*- +;;; test-local-repository--car-member.el --- Tests for localrepo--car-member -*- lexical-binding: t -*- ;;; Commentary: -;; Tests for `car-member' in local-repository.el — the predicate +;; Tests for `localrepo--car-member' in local-repository.el — the predicate ;; localrepo-initialize uses to check whether an archive id is already ;; registered in package-archives / package-archive-priorities. @@ -12,47 +12,47 @@ ;;; Normal Cases -(ert-deftest test-local-repository-car-member-found () +(ert-deftest test-local-repository-localrepo--car-member-found () "Normal: VALUE present as a car returns the matching tail (non-nil)." - (should (equal (car-member 'b '((a . 1) (b . 2) (c . 3))) + (should (equal (localrepo--car-member 'b '((a . 1) (b . 2) (c . 3))) '(b c)))) -(ert-deftest test-local-repository-car-member-not-found () +(ert-deftest test-local-repository-localrepo--car-member-not-found () "Normal: VALUE absent from every car returns nil." - (should-not (car-member 'z '((a . 1) (b . 2))))) + (should-not (localrepo--car-member 'z '((a . 1) (b . 2))))) -(ert-deftest test-local-repository-car-member-string-car () +(ert-deftest test-local-repository-localrepo--car-member-string-car () "Normal: car comparison uses `equal', so string keys match by value." - (should (car-member "localrepo" + (should (localrepo--car-member "localrepo" '(("gnu" . "url1") ("localrepo" . "url2"))))) ;;; Boundary Cases -(ert-deftest test-local-repository-car-member-empty-list () +(ert-deftest test-local-repository-localrepo--car-member-empty-list () "Boundary: an empty list never matches." - (should-not (car-member 'a nil))) + (should-not (localrepo--car-member 'a nil))) -(ert-deftest test-local-repository-car-member-single-match () +(ert-deftest test-local-repository-localrepo--car-member-single-match () "Boundary: a single-element list whose car matches returns non-nil." - (should (car-member 'only '((only . 1))))) + (should (localrepo--car-member 'only '((only . 1))))) -(ert-deftest test-local-repository-car-member-single-no-match () +(ert-deftest test-local-repository-localrepo--car-member-single-no-match () "Boundary: a single-element list whose car differs returns nil." - (should-not (car-member 'x '((only . 1))))) + (should-not (localrepo--car-member 'x '((only . 1))))) -(ert-deftest test-local-repository-car-member-nil-value-with-nil-car () +(ert-deftest test-local-repository-localrepo--car-member-nil-value-with-nil-car () "Boundary: a nil VALUE matches a cons whose car is nil." - (should (car-member nil '((nil . 1) (a . 2))))) + (should (localrepo--car-member nil '((nil . 1) (a . 2))))) -(ert-deftest test-local-repository-car-member-nil-value-no-nil-car () +(ert-deftest test-local-repository-localrepo--car-member-nil-value-no-nil-car () "Boundary: a nil VALUE with no nil car returns nil." - (should-not (car-member nil '((a . 1) (b . 2))))) + (should-not (localrepo--car-member nil '((a . 1) (b . 2))))) ;;; Error Cases -(ert-deftest test-local-repository-car-member-non-cons-element () +(ert-deftest test-local-repository-localrepo--car-member-non-cons-element () "Error: a non-cons element makes `car' signal wrong-type-argument." - (should-error (car-member 'x '(1 2)) :type 'wrong-type-argument)) + (should-error (localrepo--car-member 'x '(1 2)) :type 'wrong-type-argument)) (provide 'test-local-repository--car-member) ;;; test-local-repository--car-member.el ends here diff --git a/tests/test-system-defaults-functions.el b/tests/test-system-defaults-functions.el index 2562ff6aa..c603fc7eb 100644 --- a/tests/test-system-defaults-functions.el +++ b/tests/test-system-defaults-functions.el @@ -9,7 +9,7 @@ ;; cj/minibuffer-setup-hook -- inflate gc-cons-threshold while ;; typing in the minibuffer ;; cj/minibuffer-exit-hook -- restore gc-cons-threshold on exit -;; unpropertize-kill-ring -- strip text properties from +;; cj/--unpropertize-kill-ring -- strip text properties from ;; kill-ring at shutdown ;; cj/log-comp-warning -- route native-comp warnings to a ;; file rather than the *Warnings* @@ -79,13 +79,13 @@ (should (eq (cj/disabled) nil)) (should (commandp #'cj/disabled))) -;;; unpropertize-kill-ring +;;; cj/--unpropertize-kill-ring (ert-deftest test-system-defaults-unpropertize-kill-ring-strips-properties () "Normal: every kill-ring entry comes back with no text properties." (let ((kill-ring (list (propertize "alpha" 'face 'bold) (propertize "beta" 'face 'underline)))) - (unpropertize-kill-ring) + (cj/--unpropertize-kill-ring) (should (equal kill-ring '("alpha" "beta"))) (should-not (text-properties-at 0 (nth 0 kill-ring))) (should-not (text-properties-at 0 (nth 1 kill-ring))))) @@ -93,7 +93,7 @@ (ert-deftest test-system-defaults-unpropertize-kill-ring-boundary-empty-ring () "Boundary: an empty `kill-ring' stays empty after the strip pass." (let ((kill-ring nil)) - (unpropertize-kill-ring) + (cj/--unpropertize-kill-ring) (should (null kill-ring)))) ;;; cj/log-comp-warning diff --git a/tests/test-system-defaults.el b/tests/test-system-defaults.el index f653e1fbb..a641adea1 100644 --- a/tests/test-system-defaults.el +++ b/tests/test-system-defaults.el @@ -8,7 +8,7 @@ ;; writes land, where backups go, and whether the minibuffer GC hooks are ;; installed. Load happens in the shared sandbox (testutil-system-defaults.el). ;; -;; The module's functions (cj/disabled, the GC hook bodies, unpropertize-kill-ring, +;; The module's functions (cj/disabled, the GC hook bodies, cj/--unpropertize-kill-ring, ;; cj/log-comp-warning) are covered by test-system-defaults-functions.el, and the ;; vc-follow-symlinks default by test-system-defaults-vc-follow-symlinks.el. -- cgit v1.2.3