Re: Add elisa to GNU ELPA

[Philip Kaludercic <philipk@posteo.net>]

Sergey Kostyaev <sskostyaev@gmail.com> writes:

> Thank you for review.
>
>> 16 июля 2024 г., в 19:54, Philip Kaludercic <philipk@posteo.net> написал(а):
>> 
>> Despite
>> reading through the entire source code, I have no idea what you are
>> trying to do with this package.  It would be nice to have some more
>> context in the Commentary section and elaborate a number of user-facing
>> docstrings, unless this is intended to be expert software, for people
>> familiar with whatever the field is.
>
> I need to improve it. With README.org <http://readme.org/> it should be cleaner, but It should be understandable without it.

I wouldn't want to rely on the README.  Or at least I think of a README
file less as an introduction to the package, and more of a introduction
to the source code repository.  Having a self-contained commentary
section that just explains what the package was made for and how to use
it is usually what I like seeing the most.

> Why should we remove elisa customization group? I think there are many customization options to move it to group.

Because unless I missed something, you had only defined a single group.
By default, a `defcustom' will be added to the last group defined in
file, making it redundant to specify it again and again.  That being
said, if you prefer this explicit redundancy, feel free to keep the
:group attributes.

>> @@ -48,68 +47,61 @@
>> 					    (make-llm-ollama
>> 					     :embedding-model "nomic-embed-text"))
>>   "Embeddings provider to generate embeddings."
>> -  :group 'elisa
>> -  :type '(sexp :validate 'cl-struct-p))
>> +  :type '(sexp :validate 'cl-struct-p)) ;a more specific predicate here?
>
> It should be `llm` provider. Unfortunately there are no more specific predicate for that.

So the `llm' struct doesn't havea `llm-p' predicate?

>> (defcustom elisa-db-directory (file-truename
>> 			       (file-name-concat
>> 				user-emacs-directory "elisa"))
>>   "Directory for elisa database."
>> -  :group 'elisa
>> -  :type 'directory)
>> +  :type 'directory)                     ;is it necessary that it exists?
>
> If you mean option - then yes. If you mean directory, I can create it in `elisa' package.

I meant if you should add a :must-match t tag.

>> +     (regexp-opt (mapcar #'car reps)) ;is the last one really \0 or \\0?
>
> Really. Probably impossible case, but it’s defensive programming.

Ok!

>> +;; a number of these functions seem like something that should be added to the core of Emacs or at least a common ELPA package...
>> (defun elisa-dot-product (v1 v2)
>>   "Calculate the dot produce of vectors V1 and V2."
>
> Good idea. How can we implement it?

To my knowledge, there is no maths package in the core or ELPA (someone
correct me if I am wrong).  There is calc, but IIRC it wasn't that nice
to use from an Elisp caller.  I could imagine that just pulling out the
definition you gave here, renaming them from "elisa-foo" to "foo" would
be enough for a first daft that could be added to the core, and then to
ELPA as a core package.

>> -       (format "%s -f html --to plain"
>> -	       (executable-find elisa-pandoc-executable))
>> +       (format "%s --from html --to plain" elisa-pandoc-executable)
>>        buffer-name t)
>>       buffer-name)))
>
> We should keep executable-find call here. It was feature request from users. It can be useful with per-directory PATH, like with direnv.

I might be missing something, but resolving a executable in PATH before
invoking it shouldn't have any effect, as call-process et. AL. will
resolve the executable name in the same context as an explicit
`executable-find' does anyway.

>> 
>> (defun elisa-fts-query (prompt)
>>   "Return fts match query for PROMPT."
>> -  (thread-last
>> +  (thread-last                          ;i belive you can do all of this with a single regular expression...
>>     prompt
>>     (string-trim)
>>     (downcase)
>
> Maybe. But I prefer to keep it readable.

I don't necessarily believe it would be less readable, and now you made
me want to try it out.  I'll report back at some point.

>> @@ -1071,7 +1044,7 @@ WHERE d.rowid in %s;"
>> 		   (when-let ((kind (cl-first row))
>> 			      (path (cl-second row))
>> 			      (text (cl-third row)))
>> -		     (pcase kind
>> +		     (pcase kind        ;is this a `pcase-exhaustive'?
>> 		       ("web"
>> 			(ellama-context-add-webpage-quote-noninteractive path path text))
>> 		       ("file"
>
> I think it should be extendable, but for now it is not.

Ok.

> Best regards,
> Sergey Kostyaev
>

-- 
	Philip Kaludercic on peregrine