diff options
| -rw-r--r-- | modules/ai-config.el | 8 | ||||
| -rw-r--r-- | modules/ai-rewrite.el | 108 | ||||
| -rw-r--r-- | tests/test-ai-rewrite.el | 159 | ||||
| -rw-r--r-- | todo.org | 38 |
4 files changed, 303 insertions, 10 deletions
diff --git a/modules/ai-config.el b/modules/ai-config.el index 6eff1ba6..c7a14cae 100644 --- a/modules/ai-config.el +++ b/modules/ai-config.el @@ -35,6 +35,8 @@ (autoload 'cj/gptel-delete-conversation "ai-conversations" "Delete a saved AI conversation." t) (autoload 'cj/gptel-autosave-toggle "ai-conversations" "Toggle autosave in the current GPTel buffer." t) (autoload 'cj/gptel-quick-ask "ai-quick-ask" "One-shot quick-ask in a transient buffer." t) +(autoload 'cj/gptel-rewrite-with-directive "ai-rewrite" "Pick a directive and run gptel-rewrite on the region." t) +(autoload 'cj/gptel-rewrite-redo-with-different-directive "ai-rewrite" "Re-run the previous rewrite with a different directive." t) ;;; ------------------------- AI Config Helper Functions ------------------------ @@ -510,7 +512,8 @@ Works for any buffer, whether it's visiting a file or not." "m" #'cj/gptel-change-model ;; change the LLM model "p" #'gptel-system-prompt ;; change prompt "q" #'cj/gptel-quick-ask ;; one-shot quick ask - "r" #'gptel-rewrite ;; rewrite a region of code/text + "r" #'cj/gptel-rewrite-with-directive ;; rewrite region with a chosen directive + "R" #'cj/gptel-rewrite-redo-with-different-directive ;; redo last rewrite, new directive "c" #'cj/gptel-context-clear ;; clear all context "s" #'cj/gptel-save-conversation ;; save conversation "t" #'cj/toggle-gptel ;; toggles the ai-assistant window @@ -530,7 +533,8 @@ Works for any buffer, whether it's visiting a file or not." "C-; a m" "change model" "C-; a p" "change prompt" "C-; a q" "quick ask" - "C-; a r" "rewrite region" + "C-; a r" "rewrite region (directive)" + "C-; a R" "redo rewrite, new directive" "C-; a c" "clear context" "C-; a s" "save conversation" "C-; a t" "toggle window" diff --git a/modules/ai-rewrite.el b/modules/ai-rewrite.el new file mode 100644 index 00000000..fb25c137 --- /dev/null +++ b/modules/ai-rewrite.el @@ -0,0 +1,108 @@ +;;; ai-rewrite.el --- Directive-picker wrappers for gptel-rewrite -*- lexical-binding: t; coding: utf-8; -*- + +;; Author: Craig Jennings <c@cjennings.net> + +;;; Commentary: +;; Adds two ergonomic wrappers around `gptel-rewrite': +;; +;; cj/gptel-rewrite-with-directive Pick a named directive, +;; then rewrite the region. +;; cj/gptel-rewrite-redo-with-different-directive +;; Re-run the previous region +;; with a different directive. +;; +;; A directive is a short system-message snippet attached to a name +;; (e.g. "terse", "fix-grammar"). The directive body is injected +;; into the rewrite via `gptel-rewrite-directives-hook' just for that +;; call -- no global state changes. + +;;; Code: + +;; Declare the hook variable special so our `let'-binding below is +;; dynamic (visible across the `call-interactively' that follows) +;; rather than lexical when this file is byte-compiled. +(defvar gptel-rewrite-directives-hook) + +(defcustom cj/gptel-rewrite-directives + '(("terse" + . "Rewrite the text to be as terse as possible without losing meaning.\nDo not add commentary. Return only the rewritten text.") + ("fix-grammar" + . "Fix grammar and spelling errors only. Do not rephrase, restructure,\nor change tone. Return only the corrected text.") + ("refactor-readability" + . "Refactor the code for readability. Improve naming, split long\nfunctions when appropriate, remove unnecessary complexity, and preserve\nbehavior exactly. Return only the refactored code.") + ("add-docstring" + . "Add or improve docstrings for every function in the region. Use the\nidiomatic docstring style for the language. Do not change executable\ncode. Return the whole region with the updated docstrings.") + ("explain-as-comment" + . "Replace the region with the original code, preceded by a concise\nblock comment explaining what the code does. Use the language's\nidiomatic comment syntax. Return code + comment, nothing else.") + ("shorten" + . "Shorten the text while preserving meaning, technical accuracy, and\nthe author's voice. Remove rhetorical padding. Return only the\nshortened text.")) + "Named system-message directives for `cj/gptel-rewrite-with-directive'. +Each entry is a (NAME . BODY) pair where NAME is the directive label +presented in the completing-read prompt and BODY is the system +message injected into the next `gptel-rewrite' call." + :type '(alist :key-type string :value-type string) + :group 'cj) + +(defvar-local cj/gptel-rewrite--last-region nil + "Cons (BEG-MARKER . END-MARKER) of the last directive-driven rewrite.") + +(defvar-local cj/gptel-rewrite--last-directive nil + "Name of the directive used in the last directive-driven rewrite.") + +(defun cj/gptel-rewrite--call-with-directive (directive-name beg end) + "Run `gptel-rewrite' over BEG..END with DIRECTIVE-NAME's system message. +Stores the region (as markers) and directive name on buffer-local +variables so `cj/gptel-rewrite-redo-with-different-directive' can +revisit them." + (let ((body (alist-get directive-name cj/gptel-rewrite-directives + nil nil #'equal))) + (unless body + (user-error "Unknown rewrite directive: %s" directive-name)) + (setq-local cj/gptel-rewrite--last-region + (cons (copy-marker beg) (copy-marker end))) + (setq-local cj/gptel-rewrite--last-directive directive-name) + (let ((gptel-rewrite-directives-hook + (cons (lambda () body) gptel-rewrite-directives-hook))) + (save-excursion + (goto-char beg) + (push-mark end t t) + (call-interactively #'gptel-rewrite))))) + +;;;###autoload +(defun cj/gptel-rewrite-with-directive (directive-name) + "Pick DIRECTIVE-NAME from `cj/gptel-rewrite-directives' and rewrite the region. +Requires an active region. The directive is applied only to this +call -- it does not modify global `gptel-directives'." + (interactive + (progn + (unless (use-region-p) + (user-error "No region selected")) + (list (completing-read + "Rewrite directive: " + (mapcar #'car cj/gptel-rewrite-directives) nil t)))) + (cj/gptel-rewrite--call-with-directive + directive-name (region-beginning) (region-end))) + +;;;###autoload +(defun cj/gptel-rewrite-redo-with-different-directive () + "Re-run the previous directive-driven rewrite with a different directive. +The region is restored from the markers captured at the last call; +the user picks a new directive from the remaining choices." + (interactive) + (unless cj/gptel-rewrite--last-region + (user-error "No previous rewrite to redo in this buffer")) + (let* ((beg-mk (car cj/gptel-rewrite--last-region)) + (end-mk (cdr cj/gptel-rewrite--last-region)) + (current cj/gptel-rewrite--last-directive) + (others (cl-remove + current + (mapcar #'car cj/gptel-rewrite-directives) + :test #'equal)) + (chosen (completing-read + (format "Re-rewrite with (was %s): " current) + others nil t))) + (cj/gptel-rewrite--call-with-directive + chosen (marker-position beg-mk) (marker-position end-mk)))) + +(provide 'ai-rewrite) +;;; ai-rewrite.el ends here diff --git a/tests/test-ai-rewrite.el b/tests/test-ai-rewrite.el new file mode 100644 index 00000000..ddb83133 --- /dev/null +++ b/tests/test-ai-rewrite.el @@ -0,0 +1,159 @@ +;;; test-ai-rewrite.el --- Tests for ai-rewrite.el -*- lexical-binding: t; -*- + +;;; Commentary: +;; Tests for the directive-picker wrappers around `gptel-rewrite'. +;; `gptel-rewrite' itself is stubbed so the tests verify what the +;; wrappers do (which directive body lands in the hook, which region +;; was captured) without touching the real rewrite UI. + +;;; Code: + +(require 'ert) +(require 'cl-lib) + +(add-to-list 'load-path (expand-file-name "tests" user-emacs-directory)) +(add-to-list 'load-path (expand-file-name "modules" user-emacs-directory)) + +(require 'testutil-ai-config) + +;; Stub the gptel-rewrite surface so the wrapper can dispatch to it +;; without loading the real package. testutil-ai-config provides a +;; non-interactive stub of `gptel-rewrite'; we override it with an +;; interactive recorder that captures the hook-derived directive body +;; and the active region. +(defvar gptel-rewrite-directives-hook nil) +(defvar test-ai-rewrite--captured-directive nil + "Last system-message body produced by the hook during a stub rewrite.") +(defvar test-ai-rewrite--captured-region nil + "Cons (BEG . END) captured from `mark' and `point' at stub-rewrite time.") +(defun gptel-rewrite () + "Stub: capture the directive body and the active region." + (interactive) + (setq test-ai-rewrite--captured-directive + (run-hook-with-args-until-success 'gptel-rewrite-directives-hook)) + (setq test-ai-rewrite--captured-region + (cons (region-beginning) (region-end)))) + +(require 'ai-rewrite) + +;; ---------------------------- defcustom shape + +(ert-deftest test-ai-rewrite-directives-defcustom-has-named-entries () + "Default directives include the names called out in the spec." + (let ((names (mapcar #'car cj/gptel-rewrite-directives))) + (dolist (expected '("terse" "fix-grammar" "refactor-readability" + "add-docstring" "explain-as-comment" "shorten")) + (should (member expected names))))) + +(ert-deftest test-ai-rewrite-directives-bodies-are-strings () + "Every directive body is a non-empty string." + (dolist (entry cj/gptel-rewrite-directives) + (should (stringp (cdr entry))) + (should (> (length (cdr entry)) 0)))) + +;; ---------------------------- with-directive + +(ert-deftest test-ai-rewrite-with-directive-normal () + "Wrapper injects the directive body and runs gptel-rewrite on the region." + (with-temp-buffer + (insert "first body line\nsecond body line\n") + (let ((test-ai-rewrite--captured-directive nil) + (test-ai-rewrite--captured-region nil) + (cj/gptel-rewrite-directives + '(("test" . "BODY FOR TEST DIRECTIVE")))) + ;; Activate the region across both lines + (set-mark (point-min)) + (goto-char (point-max)) + (activate-mark) + (cj/gptel-rewrite-with-directive "test") + (should (equal test-ai-rewrite--captured-directive + "BODY FOR TEST DIRECTIVE")) + (should test-ai-rewrite--captured-region)))) + +(ert-deftest test-ai-rewrite-with-directive-error-no-region () + "No active region signals." + (with-temp-buffer + (insert "no region") + (deactivate-mark) + (should-error (call-interactively #'cj/gptel-rewrite-with-directive)))) + +(ert-deftest test-ai-rewrite-with-directive-error-unknown-directive () + "Unknown directive name signals." + (with-temp-buffer + (insert "body") + (set-mark (point-min)) + (goto-char (point-max)) + (activate-mark) + (let ((cj/gptel-rewrite-directives '(("known" . "x")))) + (should-error + (cj/gptel-rewrite--call-with-directive + "unknown" (point-min) (point-max)))))) + +(ert-deftest test-ai-rewrite-with-directive-records-last-state () + "Wrapper records the region and directive name for later redo." + (with-temp-buffer + (insert "abc\ndef\n") + (let ((cj/gptel-rewrite-directives + '(("first" . "FIRST BODY"))) + (test-ai-rewrite--captured-directive nil)) + (set-mark (point-min)) + (goto-char (point-max)) + (activate-mark) + (cj/gptel-rewrite-with-directive "first") + (should (equal cj/gptel-rewrite--last-directive "first")) + (should (consp cj/gptel-rewrite--last-region)) + (should (markerp (car cj/gptel-rewrite--last-region))) + (should (markerp (cdr cj/gptel-rewrite--last-region)))))) + +;; ---------------------------- redo + +(ert-deftest test-ai-rewrite-redo-normal () + "Redo replays the last region with a new directive." + (with-temp-buffer + (insert "line1\nline2\nline3\n") + (let* ((cj/gptel-rewrite-directives + '(("first" . "FIRST BODY") + ("second" . "SECOND BODY"))) + (test-ai-rewrite--captured-directive nil) + (test-ai-rewrite--captured-region nil)) + (set-mark (point-min)) + (goto-char (point-max)) + (activate-mark) + (cj/gptel-rewrite-with-directive "first") + (should (equal test-ai-rewrite--captured-directive "FIRST BODY")) + (let ((first-region test-ai-rewrite--captured-region)) + (setq test-ai-rewrite--captured-directive nil) + (setq test-ai-rewrite--captured-region nil) + (cl-letf (((symbol-function 'completing-read) + (lambda (_p choices &rest _) (car choices)))) + (cj/gptel-rewrite-redo-with-different-directive)) + (should (equal test-ai-rewrite--captured-directive "SECOND BODY")) + (should (equal test-ai-rewrite--captured-region first-region)))))) + +(ert-deftest test-ai-rewrite-redo-error-no-previous () + "Redo without prior rewrite signals." + (with-temp-buffer + (setq-local cj/gptel-rewrite--last-region nil) + (should-error (cj/gptel-rewrite-redo-with-different-directive)))) + +(ert-deftest test-ai-rewrite-redo-excludes-current-directive () + "Redo's completing-read prompt offers every directive except the last." + (with-temp-buffer + (insert "body") + (let ((cj/gptel-rewrite-directives + '(("a" . "A") ("b" . "B") ("c" . "C"))) + (offered nil)) + (set-mark (point-min)) + (goto-char (point-max)) + (activate-mark) + (cj/gptel-rewrite-with-directive "b") + (cl-letf (((symbol-function 'completing-read) + (lambda (_p choices &rest _) + (setq offered choices) + (car choices)))) + (cj/gptel-rewrite-redo-with-different-directive)) + (should (equal (sort (copy-sequence offered) #'string<) + '("a" "c")))))) + +(provide 'test-ai-rewrite) +;;; test-ai-rewrite.el ends here @@ -2687,14 +2687,36 @@ Survey what published gptel community tools exist (the gptel README, karthink's Output: a shortlist in =docs/design/gptel-tools-shortlist.org= with the adopt/skip/defer decisions and a follow-up extraction sub-task per "adopt". -*** TODO [#C] Promote gptel-rewrite ergonomics :feature: - -=gptel-rewrite= is the killer feature for the keep-gptel decision. Currently bound only to =C-; a r=. Make it land closer to the editing flow: - -- A directive-picker wrapper =cj/gptel-rewrite-with-directive= that =completing-read='s a directive name (=terse=, =fix-grammar=, =refactor-readability=, =add-docstring=, =explain-as-comment=, =shorten=) before delegating to =gptel-rewrite=. Directive bodies live in =ai-prompts/= so they share storage with =gptel-prompts=. -- A =cj/gptel-rewrite-redo-with-different-directive= command for "the last rewrite was close — try this style instead". Walks back to the prior region (or kept selection) and re-prompts. - -Open question: should this build on =gptel-rewrite= directly via =:after= advice + state capture, or wrap the call in a =cj/= command that sets =gptel-directives= for the duration? +*** 2026-05-16 Sat @ 01:54:34 -0500 Added directive-picker wrappers around gptel-rewrite + +New module =modules/ai-rewrite.el= with two commands: + +- =cj/gptel-rewrite-with-directive= (=C-; a r=, replacing the bare + =gptel-rewrite= binding): completing-read on a directive name from + =cj/gptel-rewrite-directives=, then rewrite the active region. +- =cj/gptel-rewrite-redo-with-different-directive= (=C-; a R=): replay + the prior region with a different directive (markers are saved + buffer-local so the region survives accept/reject of the first + rewrite). + +Open-question answer: the directive is injected via a one-shot +=let=-binding on =gptel-rewrite-directives-hook= (an abnormal hook +that gptel-rewrite already supports for per-call system messages), +not by mutating =gptel-directives= globally. No advice on +=gptel-rewrite= and no state to clean up after the call returns. + +Directives ship inline as a =defcustom= alist with the six names +called out in the task body (=terse=, =fix-grammar=, +=refactor-readability=, =add-docstring=, =explain-as-comment=, +=shorten=) so customization is straightforward without a separate +file layer. + +9 tests in =tests/test-ai-rewrite.el= cover the defcustom shape, +the wrapper (normal path, no-region error, unknown-directive +error, last-state recording), and the redo (replays prior region, +errors when no previous, excludes the current directive from the +re-pick prompt). =gptel-rewrite= stubbed for tests so no rewrite +UI fires. *** TODO [#B] Saved-conversations browser :feature: |