Re: lexical binding for emms-player-mpv

[Mike Kazantsev]

On Wed, 03 Mar 2021 11:55:13 -0500
Yoni Rabkin <yoni@rabkins.net> wrote:

> Can you please add a lexical binding declaration to the header of
> emms-player-mpv and address the compilation warnings?
> 
> This is not urgent; I'm aiming for Emms to be fully lexical-compliant by
> the May release.

Did try to do it just now, and everything there is pretty trivial
unused vars.

But there's one case which I'm not sure about, and wanted to mention it
just in case.


Issue is with this macro:

  (defmacro emms-player-mpv-cmd-prog (cmd &rest handler-body)
    ...(elided docstring here)...
    `(emms-player-mpv-cmd ,cmd (apply-partially
      (lambda (mpv-cmd mpv-data mpv-error) ,@handler-body) ,cmd)))

Intended purpose is to wrap a bunch of code into a callback easily.

For example:

  (emms-player-mpv-cmd-prog '(get_property vo)
    (let ((vo (aref mpv-data 0)))
      (message "Video output disabled: %s"
        (or (string= (alist-get 'name vo) "null") (not (alist-get 'enabled 
vo))))))

It sends `["get_property", "vo"]` command to mpv and then checks/logs
whether video output ("vo") is currently disabled in it as emacs msg.


Problem there is that I think it's impossible to reasonably use with
lexical bindings, due to optional variables it uses in that lambda.

I assume that if emms is supposed to use lexical bindings itself, all
code in it should always be usable and not raise warnings with these enabled.

It is also generally high-level part of user-facing API, so I'd rather
not break it, if at all possible.
I've found that I'm actually using it myself from my emacs config too.


Potential solutions here are:

- Just remove the macro.
- Rename vars that it defines to have underscore prefix.
- Leave it there as-is, but mark as deprecated.


Decided that I should go with the last option (mark as deprecated):

- There should be no third-party code using it with lexical bindings
  and caring about warnings, so there's no need to "fix" it in any way.

- Won't break users setup in any way, if it was used.

- Will not break "no warnings in code lexical bindings" assumption about all
  high-level EMMS APIs, but only with a caveat of "all non-deprecated ones".

- It's short and no big deal to leave around for a while.


Let me know if maybe some other option might be preferrable.


-- 
Mike Kazantsev // fraggod.net