Support adding files in directories to context

[benthamite]

Currently, attempting to add directories to the context in Dired will throw an error. This PR provides support for adding files in directories to the context: if there is a directory at point, or the marked files include one or more directories, their files will recursively be added to the context.

[karthink]

Looks good! I'll test it when I can and merge.

[benthamite]

I just noticed this in the docstring of this function (emphasis added):

If a region is selected, add the selected region to the context. If there is already a gptel context at point, remove it instead.

I may not be understanding what this means, but as far as I can tell the function does not behave in the manner described: if I select a region, add it to the context, then re-select it and call gptel-add again, the region is not removed from the context. (The docstring talks about the "context at point" rather than the region, but I am not sure what that means in this, er, context. 😛)

[karthink]

I may not be understanding what this means, but as far as I can tell the function does not behave in the manner described: if I select a region, add it to the context, then re-select it and call gptel-add again, the region is not removed from the context.

It could probably be worded better. Here is an updated description, let me know if it makes more sense:

gptel-add is an alias for ‘gptel-context-add’ in ‘gptel-context.el’.

(gptel-add &optional ARG)

Add context to gptel in a DWIM fashion.

- If a region is selected, add the selected region to the
  context.  If point is in a context region already, it
  is removed first -- overlapping context regions are not
  supported.

- 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.

- Otherwise add the current buffer to the context.  With positive
  prefix ARG, prompt for a buffer name and add it to the context.

- With negative prefix ARG, remove all gptel contexts from the
  current buffer.

[karthink]

We should also add the ability to add directories to the context when running gptel-add-file, shouldn't we?

[benthamite]

We should also add the ability to add directories to the context when running
gptel-add-file, shouldn't we?

I wasn’t familiar with this command, but yeah, it seems it should mirror the behavior of gptel-add when the latter is used with a file or directory. Would you like me to make the changes?

[benthamite]

Perhaps we should change gptel-context-add-file so that this is the function called by gptel-add in Dired mode. Currently, the former doesn’t take a prefix argument.

[karthink]

Would you like me to make the changes?

Please go ahead.

Perhaps we should change gptel-context-add-file so that this is the function called by gptel-add in Dired mode

gptel-context-add-file is the function called by gptel-add in Dired mode. There is no other way to add files to the context list.

[benthamite]

@karthink, I undid the previous attempt and pushed a new version, which (1) works with both gptel-add-file and gptel-add and (2) provides the functionality to both add and remove files in directories. Let me know what you think.

[karthink]

Will take a look soon, thanks for the update!

[karthink]

gptel-context--handle-directory is a non-interactive function. Running an interactive y-or-n-p check inside a non-interactive function is not a good idea, since gptel-add can be used non-interactively for scripting. Here's an example. So this check should happen in gptel-context-add-file.

The other advantage is that we only need to ask once per call to gptel-add, which is less annoying than asking per-directory.

[benthamite]

The other advantage is that we only need to ask once per call to gptel-add, which is less annoying than asking per-directory.

Apologies if I’m misunderstanding something, but since gptel-add calls gptel-context-add-file once per directory, moving the logic to the latter would still prompt once per directory rather than once per gptel-add call, I think. Can you clarify?

[karthink]

You are correct, I think I misunderstood something more basic about your proposed changes:

1. You are not adding all files inside a directory recursively -- instead you're only descending one level, and ignoring directories below that. Is this the behavior we want? I thought you wanted to add all files inside a directory -- why stop at the immediate children? For example, see Adding context files recursively from a directory #513.
2. On the other hand, if we do it this way, users will naturally want to exclude certain directories, like .git. If we use a rule where we ignore dotfiles, we'll also have to make an exception to that for when users want to send a configuration file .foo. They'll also want to see the tree view in the context inspection buffer instead a flat list of file contents, and we'll end up having to reimplement magit-section or part of dired, which gets messy very fast.

So there's a more important issue than where to run the y-or-n-p here. What do you think we should do?

[karthink]

We can add an &optional confirm-dir argument to this function that the (interactive ...) form can set to t.

[karthink]

Unless conirm-dir is nil, we can run the y-or-n-p check here before calling gptel-context--handle-directory.