Restructuring #2
Conversation
There was a problem hiding this comment.
Thanks so much! One other nitpick—would you be willing to edit the commit messages to adhere to the format of the previous commits (based on Conventional Commits)?
|
These all sound good and I’ll implement them later, although for the commit
messages, I’d have a slight preference for adopting the GNU ChangeLog
style, mainly out of laziness - that style is what Emacs and nearby
packages (gptel, …) use, and the built-in support for drafting such
messages is good (log-edit-generate-changelog-from-diff, etc), while I
wouldn’t know how to be comparably lazy with the conventions you mention -
do you?
…On Sat, 22 Mar 2025 at 14:41, Ad ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks so much! One other nitpick—would you be willing to edit the commit
messages to adhere to the format of the previous commits (based on Conventional
Commits <https://www.conventionalcommits.org/>)?
------------------------------
In llm-tool-collection.el
<#2 (comment)>
:
> - ***@***.***)
- (push ',sym llm-tool-collection--all-tools))))
-
+ARGS is a list where each element is of the form
+
+ (ARGNAME :type TYPE :description \"DESCRIPTION\").
+
+Arguments after the special symbol `&optional' are marked with
+`:optional t`.
+
+BODY contains the function body.
+
+This macro creates a constant with the tool's specs and defines a
+function under `llm-tc/NAME' whose docstring is the value of the spec
+`:description'. The tool is then added to
+`llm-tool-collection--all-tools'."
If this is in the docstring, should we make it into a public variable (as
in, llm-tool-collection-all-tools)? I originally intended for this to be
an internal variable only (for llm-tool-collection-get-all), but I could
see how a list of the symbols could be useful too.
------------------------------
In llm-tool-collection.el
<#2 (comment)>
:
>
-(defmacro llm-tool-collection-deftool (name specs arguments &rest body)
+(defvar llm-tool-collection-after-tool-define-hook nil
The Emacs Lisp manual recommends
<https://www.gnu.org/software/emacs/manual/html_node/emacs/Hooks.html>
that we use -functions instead of -hook for "abnormal" hooks (those that
take arguments).
------------------------------
In llm-tool-collection.el
<#2 (comment)>
:
> + (setq arg-syms (reverse arg-syms)
+ arg-specs (reverse arg-specs))
+ (let* ((sym (llm-tool-collection--name-to-symbol name))
+ (name-spec (unless (plist-get specs :name)
+ `(:name ,(llm-tool-collection--make-llm-name name)))))
+ `(progn
+ (defconst ,sym
+ ***@***.***
+ ***@***.***
+ :args ,arg-specs
+ :function ,sym))
+ (defun ,sym ,arg-syms
+ ,(concat (plist-get specs :description) "\n\n"
+ "Definition generated by `llm-tool-collection'.")
+ ***@***.***)
+ (add-to-list 'llm-tool-collection--all-tools ',sym)
The docstring for add-to-list states:
This is meant to be used for adding elements to configuration
variables, such as adding a directory to a path variable
like load-path, but please do not abuse it to construct
arbitrary lists in Elisp code, where using push or cl-pushnew
will get you more efficient code.
Thus, would it be better to use cl-pushnew instead? I know you explicitly
wanted to avoid the dependency on cl-lib, but I don't think it's a big
deal, given that it would be a compile-time dependency. However, I'm
willing to defer to popular convention here—if add-to-list is commonly
used in packages, then we can follow others' example. I suppose this is
also a minor, small use, so perhaps I'm overreacting :).
—
Reply to this email directly, view it on GitHub
<#2 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APC5ZXOHZ5OONXQV7TWTH5L2VVSAFAVCNFSM6AAAAABZR4LCBWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMBYGAZTCOJTHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
To be completely honest, I didn't know this was a thing! I've been using Conventional Commits purely out of habit and experience—I'll definitely take a look at the GNU style when I have a moment. |
* llm-tool-collection.el (llm-tool-collection-deftool): Redefine macro to accept a simpler argument format and convert it internally to the required specs. Add edebug instrumentation. (read-file, list-directory, create-file, create-directory): Update all tool definitions to use the new argument specification format.
* llm-tool-collection.el (llm-tool-collection-deftool): Add support for arguments after &optional to be marked with :optional t in argument specs. (view-buffer): Add new tool for viewing buffer contents with optional offset and limit parameters.
* llm-tool-collection.el (llm-tool-collection-deftool): Use llm-tool-collection--make-llm-name when generating parameter names.
* llm-tool-collection.el: Add Imenu support for LLM tools defined with `llm-tool-collection-deftool'.
* llm-tool-collection.el (llm-tool-collection-deftool): Replace push with cl-pushnew when registering new tools in llm-tool-collection--all-tools, to avoid duplication.
* llm-tool-collection.el (llm-tool-collection--name-to-symbol) (llm-tool-collection--make-llm-name): Move these helper functions inside an eval-and-compile form to make them available during macro expansion. (llm-tool-collection-deftool): Fix variable name conflict by renaming 'name' variable to 'name-spec' for clarity. (llm-tool-collection-get-category, llm-tool-collection-get-all): Add autoload cookies. Escape apostrophes in usage examples. (view-buffer): Reformat arguments to avoid long lines.
* llm-tool-collection.el (llm-tool-collection-deftool): Use unquoted function symbol in the :function property of the generated tool spec instead of a quoted function value.
ultronozm
force-pushed
the
restructuring
branch
from
889cf31 to
9e1fe56
Compare
Very good. These are described in the CONTRIBUTE file in my Emacs installation, and maybe also yours. I'll emphasize that I'm just "following the herd" here and have no strong opinions on commit conventions (besides wanting to be lazy and fit in). |
* llm-tool-collection.el (llm-tool-collection-post-define-functions): New variable to hold functions run after tool definition. (llm-tool-collection-deftool): Call it, passing the tool's plist definition as argument.
ultronozm
force-pushed
the
restructuring
branch
from
9e1fe56 to
5c1423f
Compare
|
I made one further tweak just now, converting category strings to symbols. The rationale is that they're enum-like (belonging to some finite collection of values rather than being general strings), so it's more idiomatic (and more efficient, I suppose) to treat them as symbols rather than strings. I'll be happy to revise if you feel otherwise |
ultronozm
force-pushed
the
restructuring
branch
from
46af7be to
09681d5
Compare
|
OK, one more little tweak (and sorry for the spam). Since |
I agree that symbols would be more appropriate. However, the reason that it is a string is because
No worries—I agree, these are good changes! |
* llm-tool-collection.el (llm-tool-collection-deftool): Accept a separate docstring parameter as second argument, for both functions and their args. (read-file, list-directory, create-file, create-directory) (view-buffer): Update all tool definitions to use the new format.
ultronozm
force-pushed
the
restructuring
branch
from
a4f971e to
3858b89
Compare
OK, I see, thanks for clarifying. I've switched it back to strings. I've checked that the changes needed in One point concerning optional arguments might be worth discussing. With the LLM's, I think any of the arguments may be marked as required or optional? With Emacs, the required must come before the optional. I updated the docstring of |
ultronozm
force-pushed
the
restructuring
branch
from
775dc55 to
cf39aeb
Compare
|
One other thought that came to mind is whether it would make sense to include commands in For instance, the following code should do the trick with (defun llm-tool-collection-register-with-gptel (tool-spec)
"Register a tool defined by TOOL-SPEC with gptel.
TOOL-SPEC is a plist that can be passed to `gptel-make-tool'."
(when (featurep 'gptel)
(declare-function gptel-make-tool "gptel")
(declare-function gptel-tool-name "gptel")
(defvar gptel-tools)
(let ((tool (apply #'gptel-make-tool tool-spec)))
(setq gptel-tools
(cons tool (seq-remove
(lambda (existing)
(string= (gptel-tool-name existing)
(gptel-tool-name tool)))
gptel-tools))))))
(add-hook 'llm-tool-collection-post-define-functions
#'llm-tool-collection-register-with-gptel)It's not clear to me whether such code belongs in the README, in some docstring, as part of For my personal package ai-org-chat.el, the corresponding code is (add-hook 'llm-tool-collection-post-define-functions #'ai-org-chat-register-tool-spec) |
I made the category a string to give the user freedom to name tool groups however they wanted. While I expect that a few standard categories will arise over time, users can also have category names like
Users have different ways to tag, and not all of these can be represented as symbols. So I wasn't thinking of it as an enum. I agree that tools contributed here should be categorized using some semi-standard set (like an enum). |
It can be part of gptel if required. Unlike the more commercial offerings, we're developing this suite of functionality split across several projects (gptel, llm, llm-tool-collection, mcp) so I'm in favor of providing glue code in all these packages to make it easy for the user to integrate them. |
See also karthink/gptel#685 As an aside, for elisp tools it usually makes sense to have a function docstring that's different from the description intended for the LLM. The latter focuses on different things. For example, it can contain instructions for the LLM on which tool (or set of tools) should be called next depending on the result of this one. This helps guide chained tool use with LLMs, as in this video and the previous one in this series. |
* gptel.el (gptel-register-tool): New function to register a tool with gptel, replacing any existing tool with the same name. Following skissue/llm-tool-collection#2 (comment)
Thanks for this and your other comments. That's a good point about the different purposes of the docstrings, so I guess it'd make sense to add a further argument specifying the docstring. It's not clear to me what would be the best ergonomics for this, so I'll leave it for now. |
That's a good point. I don't think it should be an issue—is there ever a scenario where required and optional arguments need to be intermixed?
Personally, I don't think I'll be adding this to the codebase itself. To me, users that are only consuming—not iterating on—the tools here should never have a reason to be rapidly evaluating the same tool many times. They can instead add all tools of interest via functions such as
Perhaps this is naive or reductive of me, but I am of the (tentative) opinion that, in this specific case, we can simply forgo the Elisp docstring entirely. To me, the most important benefit of the conventions of an Elisp docstring is ensuring that the interface is well defined: arguments, return value, and context. I feel that LLM-friendly descriptions almost always end up being a superset of the information one would have gleaned from an Elisp docstring, since the former also requires documenting context, arguments, and the return value. However, I'm open to examples of this assumption being false. |
llm-tool-collection.el
Outdated
| "Functions called after defining a new LLM tool. | ||
| Each function is called with one argument, the tool's plist definition.") | ||
|
|
||
| ;;;###autoload |
There was a problem hiding this comment.
Is there a reason for this macro to be autoloaded? I'm not sure if there is application outside of this package—I was under the impression that this was a macro solely to simplify making definitions for us (and contributors).
There was a problem hiding this comment.
This seemed necessary at some point to get font-lock and
indentation for the deftool macros working when editing
llm-tool-collection.el before loading the package, but I haven't been able
to reproduce that issue, so I'll be happy to remove the autoload (but will
first wait for your feedback on the other commits).
There was a problem hiding this comment.
Other commits look good to me; there's a little wording I'd like to tweak, but it's not an issue at all and I'm happy to do it post-merge. Should be ready to merge after this 🎉!
There was a problem hiding this comment.
Thanks, updated, should be all set
|
Sounds good on keeping things package agnostic, and forgoing Elisp
docstrings for now.
|
* llm-tool-collection.el (llm-tool-collection-font-lock-keywords): New constant.
* llm-tool-collection.el (llm-tool-collection-deftool): Expand docs. Clarify that optional arguments are specified via the &optional keyword, rather than with `:optional t'. Add link to the tool spec.
* llm-tool-collection.el (llm-tool-collection-deftool): Reorder macro arguments to put description last, after specs and args. Expand docstring to explain SPECS and its keywords. The rationale is to more closely mimick the ordering for 'defun' and to put the :category specification on the line below the tool name, so that one can survey tools by category using C-1 M-x occur. (read-file, list-directory, create-file, create-directory) (view-buffer): Update existing tool definitions to follow the new argument order.
* llm-tool-collection.el (edit-buffer): New tool.
ultronozm
force-pushed
the
restructuring
branch
from
7b21946 to
1d70fdd
Compare
|
Thank you so much! |
3 participants
No description provided.