From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Adam Porter Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: listen Date: Sun, 25 Feb 2024 22:15:44 -0600 Message-ID: <49be5d31-043d-4776-8284-b8c4a611ec5f@alphapapa.net> References: <51bcca65-2690-4f23-98bd-d929e63d7f94@alphapapa.net> <87il2cdb17.fsf@posteo.net> <6e496679-63f0-4a30-aa6a-8402ff96a6a1@alphapapa.net> <87zfvoac8r.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="20923"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla Thunderbird Cc: emacs-devel To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Feb 26 05:16: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 1reSQA-0005E2-Fs for ged-emacs-devel@m.gmane-mx.org; Mon, 26 Feb 2024 05:16:54 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1reSPI-0001Ar-4w; Sun, 25 Feb 2024 23:16:00 -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 1reSPE-0001AF-GN for emacs-devel@gnu.org; Sun, 25 Feb 2024 23:15:57 -0500 Original-Received: from iguana.tulip.relay.mailchannels.net ([23.83.218.253]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1reSP8-0002vw-QP for emacs-devel@gnu.org; Sun, 25 Feb 2024 23:15:56 -0500 X-Sender-Id: dreamhost|x-authsender|adam@alphapapa.net Original-Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 885FC9030B8; Mon, 26 Feb 2024 04:15:47 +0000 (UTC) Original-Received: from pdx1-sub0-mail-a290.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 2ACE4900A83; Mon, 26 Feb 2024 04:15:47 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1708920947; a=rsa-sha256; cv=none; b=G8FMoj9tXOUqZMZVJowyNYslgnPCaN3c2EMGdDtK1gzfhrjMUajTq7RId8qp2phm2YwapJ EfnagDR6cFoBzE+UNEmHg1++Qdq2RPOzaDNgeHkC7gHjdzIEHUMvx5Cmc9wsuSoUv3zL5P aKiwg+MSChU/o33aUbhQprY2dgBQ8eKCgy0o3O1Nq8aBZ1020qnpeJUwHnfqpFfpmcN9oC M+r1InJzOZWRbUepJqfG0PDklmPHJx+WNOr+y7BSyTYIf9tOwNFpBUDiOtP2P5gbG0jTvw MWfkLrHNq4RpRyA6Uinu7J1ihnMWRNuDAp3plPCcX1ziPuACKOqcJMKzKseFqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1708920947; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=DB05z6w42O77rC59jX3H7qEL7M2I3FPTKyRe14xuyyA=; b=8kifmlgbF8thcSUbBKlJI2wPZJ/LvGxKgkv2BVKnjF29Hy9D6bRVGsOY+x//FqUf+ERm31 upB+wcSuHq/8dXJzKl03zKzsAqx6HufSL00Eg40EvduPN/0F98g/arMvrnp6czXiRGyeDM iCdfetAJdfUFasF2ZQVFNOIKAm8Dd+fDFC5jdCFAkwZ3A1zKKRFFVrcMikcHfSS97ObMnf sUGOUJfSAhpgON52smiQpzyZMLKE6AmhckjZhhDxn7KAuCytjkdNelDFfFMxnc3i3VPmH1 Yh7pD/EmLSlHrVZYQ9BgTtxyhBv3PW4zDPr+WVR33Mk83FO+c0+9a9mWCqinPA== ARC-Authentication-Results: i=1; rspamd-6bdc45795d-6v8jp; auth=pass smtp.auth=dreamhost smtp.mailfrom=adam@alphapapa.net X-Sender-Id: dreamhost|x-authsender|adam@alphapapa.net X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|adam@alphapapa.net X-MailChannels-Auth-Id: dreamhost X-Trade-Spill: 1bb1629068d52f9d_1708920947416_790612211 X-MC-Loop-Signature: 1708920947416:4170230968 X-MC-Ingress-Time: 1708920947416 Original-Received: from pdx1-sub0-mail-a290.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.102.40.7 (trex/6.9.2); Mon, 26 Feb 2024 04:15:47 +0000 Original-Received: from [10.179.6.34] (unknown [45.130.83.1]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: adam@alphapapa.net) by pdx1-sub0-mail-a290.dreamhost.com (Postfix) with ESMTPSA id 4TjnNL1SX5zBB; Sun, 25 Feb 2024 20:15:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alphapapa.net; s=dreamhost; t=1708920947; bh=DB05z6w42O77rC59jX3H7qEL7M2I3FPTKyRe14xuyyA=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=UwO6ODpAvCj1Oj8H4eH+k4OsvR1Zp8xQJSyAduOtCCbLm3eVvSFZIcXN7sKMbATz+ FwiYKMQVPGkePxYTs6Rw3bOVfDr/lAvwre9BIewJATiDYj6XktX/hHch0pQpMZuJJw VhGdRoQuYyAAwDmUjlluux4oeRS66Zd3l17gIXgqppi7ZKyYEl8Ert/zUjwBoMFA4b ePrUoOEAgeZJq7ZXOizmXI9Z5AdIiCtYGkMeLyXpUWp/qlExw7VSQDs3u4LMUQDCtl QGL0bf/RRJgF0o3jTWzIYV1iI2n0uuhHBRUm8m9yjnfJmWLDB+CCY3TpySD/y67UG2 wCTBMmrJvCyIg== Content-Language: en-US In-Reply-To: <87zfvoac8r.fsf@posteo.net> Received-SPF: neutral client-ip=23.83.218.253; envelope-from=adam@alphapapa.net; helo=iguana.tulip.relay.mailchannels.net X-Spam_score_int: -12 X-Spam_score: -1.3 X-Spam_bar: - X-Spam_report: (-1.3 / 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_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_NEUTRAL=0.779, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no 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:316545 Archived-At: Hi Philip, On 2/25/24 07:45, Philip Kaludercic wrote: >> 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. I'll keep it for now, because I'm not ruling out the possibility of supporting earlier versions at some point. > 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). This is why I like `rx'. :) >> 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'. Good point. I'll fix that. >> 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. Well, Elisp isn't Scheme, and CLtL2 seems to say that it is exactly a minor tradition. :) I don't like it for a few reasons: 1. Using AND puts the conditional expressions on the same level, visually and logically, as the value. In contrast, using WHEN cleanly separates the condition(s) from the value. IMO that's a significant advantage, as it makes the purpose of the code much clearer. 2. WHEN's indentation also saves space, helping to avoid long lines or awkwardly wrapped ones. > There were a few other places where you did (delq nil (mapcar ...)) that > might be replaced by this pattern. I confess that I've hardly ever used MAPCAN in any of my code--still, I'm not sure how that would help to avoid that pattern. For example: (let ((a '(1 nil 2 nil 3)) (b '(4 nil 5 nil 6))) (mapcan #'identity (list a b))) ;; (1 nil 2 nil 3 4 nil 5 nil 6) But I'm probably missing something. > 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. I do care about compiler warnings--I wrote makem.sh to catch such warnings in my projects before they reach users--but in this case, that code's not going anywhere, because it's already with bookmark-related code. > Ah, the `t did confuse me momentarily, but in that case you can replace > the (guard ...) with (and 't (guard ...)). As much as I advocate using pcase and its powerful expressions, I think that would make this example harder to follow. The pcase pattern is used to test an argument, and the string test is a separate concern. >>> @@ -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. It's used in two places, and it would be undesirable to recalculate the list's length every time through the loop. > 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. Handling the case of files being renamed "out from under" the queue seems like a non-trivial problem to solve well. The `listen-library' command and view record the initial paths used, i.e. both directories and files, but the queue only records filenames, so if those are changed, there's not a reliable way to rediscover the renamed files. AFAIK other music library software handles it by removing the now-missing files from its database and rescanning the whole directory tree to find new files--but even that wouldn't replace tracks in a playlist if their underlying files disappear. `listen' doesn't keep a database, just queues of tracks. So I don't know if this particular problem is reasonably solvable. (Again, it can only happen if the user intentionally renames files during playback, which I discovered while tidying up the metadata on some of my files.) > Formatting. Emacs highlights the "nil t" as occurring in-between closed > parentheses, since it can be easily missed. Yes, but in this case, the "nil t" is a common enough pattern that I'm willing to allow it to be a "trailer" to avoid "nil t" on a line by themselves. >> "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. Good idea. I'll add that. >> 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-") ;=> 31 (#o37, #x1f, ?\C-_) > (string-to-number "x") ;=> 0 (#o0, #x0, ?\C-@) > (string-to-number "") ;=> 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 stringp 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. It could be more robust, yes. Note that this function is only passed a string read from the user and returns an integer, so if it fails, it's not a big deal. I'll think about how to make it fail more usefully... >>> 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. I don't know about that. Especially for small packages with trivial documentation. Maintaining documentation and commentaries, keeping them reasonably in sync, etc. is enough work without having them split into multiple files. Having a README.org which is viewable at the package's repo also generate the manual is a relief to me. >>> Also, your README includes this line >>> :vc (:fetcher github :repo "alphapapa/listen.el") >>> which is malformed. >> >> I just tested that, and it works for me. > > On Emacs 30? That is not the code we merged... No, I'm using Emacs 29 with `vc-use-package'. Its documentation seems to suggest that it uses the same format as that merged into Emacs 30, since it says that its features were merged into Emacs 30. Maybe `vc-use-package's documentation should be updated to reflect this? >>> What you want is >>> :vc (:url "https://github.com/alphapapa/listen.el") Ok, so no ".git" on the end (i.e. relying on the GitHub redirect)? And does this mean that none of the host-specific "fetchers" are available in Emacs 30? (Which FTR is fine with me, as the URL should be enough, I'm just curious.) --Adam