On Fri, 13 Apr 2018 15:23:05 -0400 Ian Dunn <du...@gnu.org> wrote: > 1a. You should require s and dash in the header, as they both appear to be > dependencies. I can't immediately see any others. > 1b. Neither is currently a dependency of EMMS, so either they'd have to be > added, or removed from your code. > > 2. In "emms-mpv-ipc-line", give the option for users to introduce their own > event handlers. This makes it easier to extend to support additional events. > > 3. As stated in another email, you use the --idle flag, which seems to exist > for this very purpose. Thanks for that. > > 4. A general convention when dealing with external processes in emacs is to > break up command line args and the actual program. It especially helps when > users want to use a specific binary. Otherwise, I'd have to modify just the > first argument of a list if I wanted to modify it at all in a robust way. > > 5. Adding to 4, I'd recommend making emms-mpv-proc-cmd a defcustom instead of > a defvar.
Completely agree. Definitely need to make quite a few defvar's there into defcustom's, including separate binary/args. s/dash deps (pretty sure I use cl-return and let* too) are only omitted because it's in the repo where I use them all over the place, and would be better to drop them entirely for emms code to avoid new requires there. Will be sure to address all these, thanks a lot for pointing them out! -- Mike Kazantsev // fraggod.net _______________________________________________ Emms-help mailing list Emms-help@gnu.org https://lists.gnu.org/mailman/listinfo/emms-help
