-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
6b2c0b9
c99b2cc
147fe6e
c45ab15
8bee48a
bc9138a
6c66982
2a40dae
8e5e3c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
the file at point is a directory, or directories are marked, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is difficult to read. How about
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I missing something here? Seems like this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convert to a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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) | ||
|
There was a problem hiding this comment.
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 withgptel-add
?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)