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