Re: [ELPA] New package: listen

[Adam Porter]

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