Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support adding files in directories to context #438

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
69 changes: 49 additions & 20 deletions gptel-context.el
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,10 @@ context chunk. This is accessible as, for example:
context. If there is already a gptel context at point, remove it
instead.

- If in Dired, add marked files or file at point to the context.
With negative prefix ARG, remove them from the context instead.
- If in Dired, add marked files or file at point to the context. If
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should ask for confirmation (y-or-n-p) when adding a directory with gptel-add?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I weakly lean towards asking for confirmation. The prompt could include the number of files to be added.

Copy link
Contributor Author

@benthamite benthamite Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s an attempt: 8e5e3c8

Perhaps we should add a user option to allow the user to decide if they want to be asked for confirmation.

Also, note that the user will be asked once for each directory. This seems like the correct behavior, though it can become tedious if the user is adding several dirs. (Perhaps this provides an additional reason for adding the user option.)

the file at point is a directory, or directories are marked,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is difficult to read. How about

- If in Dired, add marked files or file at point to the context.  Directories
  will have all their files added recursively.  With negative prefix ARG, remove
  them from the context instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Copy link
Contributor Author

@benthamite benthamite Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: 8bee48a

recursively add all their files. With negative prefix ARG, remove
them from the context instead.

- Otherwise add the current buffer to the context. With positive
prefix ARG, prompt for a buffer name and add it to the context.
Expand Down Expand Up @@ -163,32 +165,57 @@ context chunk. This is accessible as, for example:
(eq buffer-file-coding-system 'no-conversion))
(file-missing (message "File \"%s\" is not readable." path))))

(defun gptel-context--handle-binary (path)
"Add binary file at PATH to context if supported.
Return PATH if added, nil if ignored."
(if-let* (((gptel--model-capable-p 'media))
(mime (mailcap-file-name-to-mime-type path))
((gptel--model-mime-capable-p mime)))
(prog1 path
(cl-pushnew (list path :mime mime)
gptel-context--alist :test #'equal)
(message "File \"%s\" added to context." path))
(message "Ignoring unsupported binary file \"%s\"." path)
nil))

(defun gptel-context--handle-directory (path action)
"Process all files in directory at PATH according to ACTION.
ACTION should be either `add' or `remove'."
(let ((files (directory-files-recursively path "." t)))
(mapc (lambda (file)
(unless (file-directory-p file)
(pcase action
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pcase-exhaustive to future-proof uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: 6c66982

('add (gptel--file-binary-p file)
(gptel-context--handle-binary file)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something here? Seems like this should be (if (gptel--file-binary-p file) (gptel-context--handle-binary file) ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, not sure what happened. Will make these and the other changes shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: 2a40dae

(cl-pushnew (list file) gptel-context--alist :test #'equal)
(message "File \"%s\" added to context." file))
('remove
(setf (alist-get file gptel-context--alist nil 'remove #'equal) nil)
(message "File \"%s\" removed from context." file)))))
files)))

(defun gptel-context-add-file (path)
"Add the file at PATH to the gptel context.

If PATH is a directory, recursively add all files in it.
PATH should be readable as text."
(interactive "fChoose file to add to context: ")
(if (gptel--file-binary-p path) ;Attach if supported
(if-let* (((gptel--model-capable-p 'media))
(mime (mailcap-file-name-to-mime-type path))
((gptel--model-mime-capable-p mime)))
(prog1 path
(cl-pushnew (list path :mime mime)
gptel-context--alist :test #'equal)
(message "File \"%s\" added to context." path))
(message "Ignoring unsupported binary file \"%s\"." path))
;; Add text file
(cl-pushnew (list path) gptel-context--alist :test #'equal)
(message "File \"%s\" added to context." path)
path))
(if (file-directory-p path)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert to a cond clause, it's easier to read here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: bc9138a

(gptel-context--handle-directory path 'add)
(if (gptel--file-binary-p path)
(gptel-context--handle-binary path)
;; Add text file
(cl-pushnew (list path) gptel-context--alist :test #'equal)
(message "File \"%s\" added to context." path)
path)))

;;;###autoload (autoload 'gptel-add-file "gptel-context" "Add files to gptel's context." t)
(defalias 'gptel-add-file #'gptel-context-add-file)

(defun gptel-context-remove (&optional context)
"Remove the CONTEXT overlay from the contexts list.
If CONTEXT is nil, removes the context at point.
If selection is active, removes all contexts within selection."
If selection is active, removes all contexts within selection.
If CONTEXT is a directory, recursively removes all files in it."
(cond
((overlayp context)
(delete-overlay context)
Expand All @@ -198,9 +225,11 @@ If selection is active, removes all contexts within selection."
for ov in (alist-get (current-buffer) gptel-context--alist)
thereis (overlay-start ov))
(setf (alist-get (current-buffer) gptel-context--alist nil 'remove) nil)))
((stringp context) ;file
(setf (alist-get context gptel-context--alist nil 'remove #'equal)
nil))
((stringp context) ;file or directory
(if (file-directory-p context)
(gptel-context--handle-directory context 'remove)
(setf (alist-get context gptel-context--alist nil 'remove #'equal)
nil)))
((region-active-p)
(when-let ((contexts (gptel-context--in-region (current-buffer)
(region-beginning)
Expand Down