Re: Add elisa to GNU ELPA

[Sergey Kostyaev <sskostyaev@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

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.

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

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

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

> +     (regexp-opt (mapcar #'car reps)) ;is the last one really \0 or \\0?

Really. Probably impossible case, but it’s defensive programming.

> +;; 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?

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

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

Best regards,
Sergey Kostyaev


[-- Attachment #2: Type: text/html, Size: 7120 bytes --]