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: [ELPA] New package: listen Date: Sun, 25 Feb 2024 13:45:40 +0000 Message-ID: <87zfvoac8r.fsf@posteo.net> References: <51bcca65-2690-4f23-98bd-d929e63d7f94@alphapapa.net> <87il2cdb17.fsf@posteo.net> <6e496679-63f0-4a30-aa6a-8402ff96a6a1@alphapapa.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="13143"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel To: Adam Porter Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Feb 25 14:46:54 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 1reEqA-00033Z-QJ for ged-emacs-devel@m.gmane-mx.org; Sun, 25 Feb 2024 14:46:51 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1reEpU-0007Le-1V; Sun, 25 Feb 2024 08:46:08 -0500 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 1reEpC-0007CZ-Gq for emacs-devel@gnu.org; Sun, 25 Feb 2024 08:45:59 -0500 Original-Received: from mout02.posteo.de ([185.67.36.66]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1reEp8-0006sP-Op for emacs-devel@gnu.org; Sun, 25 Feb 2024 08:45:50 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 087B6240101 for ; Sun, 25 Feb 2024 14:45:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1708868742; bh=DScqX2OxWpScdh2w8aqGRKUxIAGShLNjhYoXGPoS4o0=; h=From:To:Cc:Subject:Autocrypt:Date:Message-ID:MIME-Version: Content-Type:Content-Transfer-Encoding:From; b=NTANCJUldxjB8oEXZ/zupVUr2PcWd2DSLpSteUFhO4zgG1l6ZMk9VLTJNNsO1yCBs YCN3pypIA8ZcjSiQaOIHedi+fKTcu2QzCvAWheCcL6BreJSFSfbHnQVU34t+rnIsyO eHA+J6Ta/OIAN58EylgKMspzMzEbbnKHVtC1t2gBJ5veWGSy1Rt6EGhiKO+KO6xgks I7MYp0paIXrmwMr+RUrzZez7erv0JzahJY0H1XV5AuU7YI53/VpeZm2ZtlffTVSQI+ /zyiOWE8VWprJSRcHfhVvpm2+xQX8RsGsmJ6aU6dGz8qCCYqJETtLSWwqjJGGFKgMI oKFNlSeLuRCkA== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4TjQ4N75grz9rxK; Sun, 25 Feb 2024 14:45:40 +0100 (CET) In-Reply-To: <6e496679-63f0-4a30-aa6a-8402ff96a6a1@alphapapa.net> (Adam Porter's message of "Sun, 25 Feb 2024 07:14:59 -0600") X-Hashcash: 1:20:240225:emacs-devel@gnu.org::ga2s7cD83rs/OUoz:2qTM X-Hashcash: 1:20:240225:adam@alphapapa.net::l5+LRJY6Pel24frV:3vIM Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM Received-SPF: pass client-ip=185.67.36.66; envelope-from=philipk@posteo.net; helo=mout02.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.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 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:316518 Archived-At: Adam Porter writes: > Hi Philip, > > Thanks for your review. Comments to follow: > > On 2/25/24 05:43, Philip Kaludercic wrote: >> Adam Porter writes: >>=20 >>> Hi, >>> >>> I'd like to submit a new package to ELPA, named "Listen". It's a >>> simple audio/music player that uses VLC as a backend. >>> >>> https://github.com/alphapapa/listen.el >> Note that the copyright lines still have to be updated for it to be >> added to GNU ELPA. > > Oops, I forgot to update the ones other than in listen.el. Done. > >> Here are a few comments, questions and suggestions from briefly skimming >> the code: >> diff --git a/listen-info.el b/listen-info.el >> index e74d376df9..a95a5b1d1b 100644 >> --- a/listen-info.el >> +++ b/listen-info.el >> @@ -56,7 +56,6 @@ >> (require 'bindat) >> (require 'cl-lib) >> -(require 'seq) > > The `seq' library is used in the file, so shouldn't this line be present? Sorry, should have explained that. If you depend on Emacs 29, then seq is preloaded, so you don't need the line. If you prefer to, you can keep it in though. >> @@ -942,10 +941,10 @@ extension. >> Return one of symbols `vorbis', `opus', `flac', or `mp3'." >> (let ((case-fold-search t)) >> - (cond ((string-match ".ogg$" filename) 'vorbis) >> - ((string-match ".opus$" filename) 'opus) >> - ((string-match ".flac$" filename) 'flac) >> - ((string-match ".mp3$" filename) 'mp3) >> + (cond ((string-match ".ogg\\'" filename) 'vorbis) >> + ((string-match ".opus\\'" filename) 'opus) >> + ((string-match ".flac\\'" filename) 'flac) >> + ((string-match ".mp3\\'" filename) 'mp3) >> (t nil)))) > > According to the Elisp manual: "When matching a string instead of a > buffer, =E2=80=98$=E2=80=99 matches at the end of the string or before a = newline > character." So it appears to be correct here (and its meaning is > widely known, whereas the Emacsism "\\'" is not (I can never remember > the escape sequences). Hmm, I just went by the convention used in auto-mode-alist here (the trick to remember \\' is that \\' and \\' always occur in the same order as you would quote a symbol in a docstring). > A good change to make, though, would be to escape the period, as > otherwise it would match any character, as in "fooogg" rather than > "foo.ogg". Right. >> diff --git a/listen-lib.el b/listen-lib.el >> index 7743080c23..b3d48f51fa 100644 >> --- a/listen-lib.el >> +++ b/listen-lib.el >> @@ -50,7 +50,7 @@ >> ;;;; Faces >> (defgroup listen-faces nil >> - "Faces." >> + "Faces used by listen.el." >> :group 'listen) > > As the `listen-faces' group is within the `listen' group, I would > think that that its faces are related to `listen' goes without saying. But that context would be missing if you were to use something like `customize-apropos-groups'. >> diff --git a/listen-library.el b/listen-library.el >> index 77022e1791..90d40468ad 100644 >> --- a/listen-library.el >> +++ b/listen-library.el >> @@ -100,8 +100,8 @@ mode line and bookmark name. BUFFER may be specifie= d in which to >> show the view." >> (interactive >> (list (list (read-file-name "View library for: ")) >> - :name (when current-prefix-arg >> - (read-string "Library name: ")))) >> + :name (and current-prefix-arg >> + (read-string "Library name: ")))) > > No, thanks. I'm aware of this minor tradition, but I disagree with it. I wouldn't call it a minor tradition (e.g. in Scheme the return value of `when' is unspecified. CLtL2 says "If the value is relevant, then it may be stylistically more appropriate to use and or if."), and I am curious why you disagree with it, but you are of course free to do as you want. >> @@ -181,7 +181,7 @@ Interactively, read COMMAND and use tracks at point = in >> ;; it ought to return sections in the region), it returns nil. >> ;; This may be confusing to users, but it seems like an upstream >> ;; issue. >> - (or (flatten-list (mapcar #'value-of (magit-region-sections))) >> + (or (flatten-list (mapcar #'value-of (magit-region-sections))) ;or = `mapcan'? >> (value-of (magit-current-section))))) > > Probably a good idea. There were a few other places where you did (delq nil (mapcar ...)) that might be replaced by this pattern. >> @@ -192,7 +192,7 @@ Interactively, read COMMAND and use tracks at point = in >> "Return a bookmark record for the current library buffer." >> (cl-assert listen-library-paths) >> `(,(format "Listen library: %s" (or listen-library-name listen-libra= ry-paths)) >> - (handler . listen-library--bookmark-handler) >> + (handler . ,#'listen-library--bookmark-handler) > > Is there any advantage to this, given that the function is defined a > few lines later? Compiler warnings don't seem relevant here. Not really, if you don't care about compiler warnings. It just seems like the kind of things that could cause problems at some later time, when definitions are moved around. >> diff --git a/listen-mpd.el b/listen-mpd.el >> index b8a104febe..8c3a302f08 100644 >> --- a/listen-mpd.el >> +++ b/listen-mpd.el >> @@ -89,17 +89,17 @@ completion." >> (cons 'affixation-function #'aff= ix) >> ;; (cons 'annotation-function #'= annotate) >> )))) >> - (`t (unless (string-empty-p str) >> - (let ((tag (pcase tag >> - ('any 'file) >> - (_ tag)))) >> - (delete-dups >> - (delq nil >> - (mapcar (lambda (row) >> - (when-let ((value (alist-get= tag row))) >> - (propertize value >> - :mpc-alist row= ))) >> - (search-any (split-string str)= ))))))))) >> + ((guard (not (string-empty-p str))) >> + (let ((tag (pcase tag >> + ('any 'file) >> + (_ tag)))) >> + (delete-dups >> + (delq nil >> + (mapcar (lambda (row) >> + (when-let ((value (alist-get tag = row))) >> + (propertize value >> + :mpc-alist row))) >> + (search-any (split-string str))))))= ))) > > AFAICT, this change would not be strictly equivalent, as the original > tests explicitly for t before testing the string, which, AFAICT, is > the correct way to handle this part of the completion API. Ah, the `t did confuse me momentarily, but in that case you can replace the (guard ...) with (and 't (guard ...)). >> diff --git a/listen-queue.el b/listen-queue.el >> index 9515e3d7dc..cf4bd494bd 100644 >> --- a/listen-queue.el >> +++ b/listen-queue.el >> @@ -312,7 +312,7 @@ PROMPT is passed to `format-prompt', which see." >> ;; homedir path (so queues could be portable with music >> ;; libraries). >> :filename (abbreviate-file-name filename) >> - :artist (map-elt metadata "artist") >> + :artist (map-elt metadata "artist") ;`metadata' is an alist, >> right? So why not use `let-alist'? > > Please note that the keys are strings, which AFAICT `let-alist' is not > compatible with (nor `pcase-let', as it does not allow the test > function to be specified). `map-elt' tests with `equal'. OK, I see. >> @@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see." >> n) >> (when current-track >> (cl-callf2 delete current-track tracks)) >> - (setf n (length tracks)) >> + (setf n (length tracks)) ;why the variable? > > Because the value is used elsewhere in the function. Am I missing > something? (Anyway, as noted in the source, I did not write that > function.) Then I missed something, because I just saw the variable being declared in the let-head, set here and used once later. >> @@ -357,6 +357,8 @@ PROMPT is passed to `format-prompt', which see." >> (1+ (seq-position (listen-queue-tracks queue) >> (listen-queue-current queue) >> (lambda (a b) >> + ;; FWIW you still have a similar >> + ;; race condition here. > > I don't understand what you mean. The comment before this hunk of > code is not about a race condition (not a subtle or random one, > anyway--it's a matter of whether the user manually modifies a track > and then refreshes the queue's tracks from disk while the track is > playing, something that is unlikely to happen unless the user is > actively tidying their tracks' metadata, and fixing it will require > more careful consideration; as well, if the file gets renamed in the > process, there's little that can be done, as the track's primary > identifier will no longer exist). My understanding was that you were handling the case where files could be renamed, but if that is not your concern, then disregard the comment. >> @@ -427,7 +429,7 @@ tracks in the queue unchanged)." >> (goto-char beg) >> (cl-loop collect (vtable-current-object) >> do (forward-line 1) >> - while (<=3D (point) end)))))) >> + until (eobp)))))) > > This code does not test for the end of the buffer, so `eobp' would not > be appropriate. You are right, never mind. >> --- a/listen-vlc.el >> +++ b/listen-vlc.el >> @@ -43,7 +43,8 @@ >> (save-excursion >> (insert (listen--send player "info"))) >> (cl-loop while (re-search-forward (rx bol "| " (group (1+ (not bla= nk))) ": " >> - (group (1+ (not (any ""))))) = nil t) >> + (group (1+ (not (any "\C-m"))= ))) >> + nil t) > > Other than the formatting, what is the purpose of this change? This > function parses output from VLC, and it appears to work correctly. Formatting. Emacs highlights the "nil t" as occurring in-between closed parentheses, since it can be easily missed. >> @@ -116,7 +117,7 @@ Stops playing, clears playlist, adds FILE, and plays= it." >> "Return or set PLAYER's VOLUME. >> VOLUME is an integer percentage." >> (if volume >> - (listen--send player (format "volume %s" (* 255 (/ volume 100.0))= )) >> + (listen--send player (format "volume %s" (* 255 (/ volume 100.0))= )) ;what if this is OOB? > > "Out Of Bounds"? What do you mean? VLC returns a value from 0-255, > and `volume' is specified to be an integer percentage (i.e. from > 0-100). As far as I can tell, this works correctly. I couldn't infer that from reading the function, so I wondered what happens when `volume' is not between 0 and 100. Perhaps a cl-assert would be nice to have. >> diff --git a/listen.el b/listen.el >> index 3e3d11d572..c381b2a113 100755 >> --- a/listen.el >> +++ b/listen.el >> @@ -75,7 +75,7 @@ >> :link '(emacs-commentary-link "listen") >> :link '(emacs-library-link "listen")) >> -(defcustom listen-directory "~/Music" >> +(defcustom listen-directory "~/Music" ;perhaps check XDG_MUSIC_DIR he= re? > > I cannot find such a variable defined, neither on my system nor on a > Web search. It doesn't appear to be part of the XDG standard. > >> -(declare-function listen-queue-next "listen-queue") >> +(declare-function listen-queue-next "listen-queue") ;why not `require' = it? > > Because it is an autoloaded command, one which the user might not use, > so loading the library might be unnecessary. > >> @@ -251,19 +251,19 @@ command with completion." >> (defun listen-read-time (time) >> "Return TIME in seconds. >> TIME is an HH:MM:SS string." >> - (string-match (rx (group (1+ num)) >> - (optional ":" (group (1+ num)) >> - (optional ":" (group (1+ num))))) >> - time) >> - (let ((fields (nreverse >> - (remq nil >> - (list (match-string 1 time) >> - (match-string 2 time) >> - (match-string 3 time))))) >> - (factors [1 60 3600])) >> - (cl-loop for field in fields >> - for factor across factors >> - sum (* (string-to-number field) factor)))) >> + (when (string-match (rx (group (1+ num)) >> + (optional ":" (group (1+ num)) >> + (optional ":" (group (1+ num))))) >> + time) >> + (let ((fields (nreverse >> + (remq nil >> + (list (match-string 1 time) >> + (match-string 2 time) >> + (match-string 3 time))))) >> + (factors [1 60 3600])) >> + (cl-loop for field in fields >> + for factor across factors >> + sum (* (string-to-number field) factor))))) > > If this function fails, I want it to signal an error, not return nil. Am I missing something, or where will the error be signaled? If the pattern doesn't match, the match data won't be modified, and you'll extract arbitrary substrings out of TIME. `String-to-number' doesn't raise an error on malformed input, (string-to-number "31-") ;=3D> 31 (#o37, #x1f, ?\C-_) (string-to-number "x") ;=3D> 0 (#o0, #x0, ?\C-@) (string-to-number "") ;=3D> 0 (#o0, #x0, ?\C-@) the only exception being if there was no match data for some n (string-to-number (match-string 100)) ;signals (wrong-type-argument strin= gp nil) If you want to signal an error, then I think the robust thing would be to check if `string-match' succeeds as proposed above, but using an `if' not a `when' to raise an error in the ELSE case. >>> For more details (though there's not much else to say), please see the >>> readme/Info manual. >> On the topic of the readme/manual, wouldn't it be better to have a >> separate README file? Then again, the manual is pretty short, and I >> don't know if it is worth having it in the first place... > > As you said, this readme is currently trivial. Were it larger, > however--well, I have other packages with much larger readmes that are > also converted to manuals. There would not be much gained by > separating into files. I don't think that is a good practice. A README for when you have fetched the sources and want to figure out what is what, a manual for when you have already installed a package and a package description for when you are previewing a package using C-h P are three different things. One shouldn't cover all of it with the same file if you ask me, since they all have different audiences with different levels of interest. >> Also, your README includes this line >> :vc (:fetcher github :repo "alphapapa/listen.el") >> which is malformed.=20=20 > > I just tested that, and it works for me. On Emacs 30? That is not the code we merged... --8<---------------cut here---------------start------------->8--- Debugger entered--Lisp error: (error "use-package: Keyword :vc received unk= nown argument: :fetcher. Supported keywords are: (:url :branch :lisp-dir :m= ain-file :vc-backend :rev)") signal(error ("use-package: Keyword :vc received unknown argument: :fetch= er. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend = :rev)")) error("use-package: %s" "Keyword :vc received unknown argument: :fetcher.= Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :re= v)") use-package-normalize--vc-arg((listen :fetcher github :repo "alphapapa/li= sten.el")) use-package-normalize/:vc(listen :vc ((:fetcher github :repo "alphapapa/l= isten.el"))) use-package-normalize-plist(listen (:vc (:fetcher github :repo "alphapapa= /listen.el")) nil use-package-merge-keys) use-package-normalize-keywords(listen (:vc (:fetcher github :repo "alphap= apa/listen.el"))) ... --8<---------------cut here---------------end--------------->8--- >> What you want is >> :vc (:url "https://github.com/alphapapa/listen.el") >> or after the package has been added to ELPA, then just >> :vc t > > I added that to the readme for before the package is available through > ELPA. Afterward I intend to recommend installing it from ELPA, not > from git. Yes, I noticed that after I had sent my message. That is better practice anyway, at least for the regular user. > Please let me know if any other changes are needed. > > --Adam > --=20 Philip Kaludercic on icterid