From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: Add elisa to GNU ELPA Date: Tue, 16 Jul 2024 16:04:17 +0000 Message-ID: <87wmllqq66.fsf@posteo.net> References: <8734o9sdig.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33027"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Sergey Kostyaev Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Jul 16 18:05:31 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1sTkgD-0008Dh-EV for ged-emacs-devel@m.gmane-mx.org; Tue, 16 Jul 2024 18:05:29 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sTkfT-0001Im-DF; Tue, 16 Jul 2024 12:04:44 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sTkfD-0000UO-Qh for emacs-devel@gnu.org; Tue, 16 Jul 2024 12:04:28 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sTkf8-0001IO-SZ for emacs-devel@gnu.org; Tue, 16 Jul 2024 12:04:27 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id D2C7024002B for ; Tue, 16 Jul 2024 18:04:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1721145858; bh=yH6mM5MRpB0w9qhman/xFVCPMCgwZwflou11yJI7e5Y=; h=From:To:Cc:Subject:OpenPGP:Date:Message-ID:MIME-Version: Content-Type:Content-Transfer-Encoding:From; b=g9dl8znI2+yosQZ0OXjh9wXEhJT5YB+NyOzwaUr6aHK5UhMauFPaIdEeESqyVNIwe TVwRDJBv0Ujr28c4iASvL1CDyzvdocuqJnyr0WfCTiHji7Ig+zbqvRVMIYAC/TdNL0 p3MuCo1orXt+5/WAk+lOxhe1+++7hXcamFOCN3/YenB5YY916BFysQ/ayO3XyWz0Oz 5x/VzaajtZBiKGluuRDc4sLSxBz0xvMWXfd67Ek+tgo505zHeOIBAuq5YQmKdbqLVP G4SL/R+b/m0IgKC01ESh9bJ9fHYHVtcnCtyoiGlfvMUSd4gjywrWh1V40pOh5SacTV nWblws8zmxKoQ== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4WNkQp2727z9rxF; Tue, 16 Jul 2024 18:04:17 +0200 (CEST) In-Reply-To: (Sergey Kostyaev's message of "Tue, 16 Jul 2024 20:57:41 +0700") OpenPGP: id=7126E1DE2F0CE35C770BED01F2C3CC513DB89F66; url="https://keys.openpgp.org/vks/v1/by-fingerprint/7126E1DE2F0CE35C770BED01F2C3CC513DB89F66"; preference=signencrypt Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:321719 Archived-At: Sergey Kostyaev writes: > Thank you for review. > >> 16 =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2 19:54, Philip K= aludercic =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(= =D0=B0): >>=20 >> 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 c= leaner, 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 cu= stomization 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 pre= dicate 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=E2=80=99s 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 use= rs. 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. >>=20 >> (defun elisa-fts-query (prompt) >> "Return fts match query for PROMPT." >> - (thread-last >> + (thread-last ;i belive you can do all of thi= s 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 > --=20 Philip Kaludercic on peregrine