search

jcaw / theme-magic

🎨 Apply your Emacs theme to the rest of Linux, using magic. Also works on Mac.
GNU General Public License v3.0
143 stars 4 forks source link

Melpa suggestions #1

Closed jcaw closed 5 years ago

jcaw commented 5 years ago

Implement MELPA pull request suggestions, from @riscy https://github.com/melpa/melpa/pull/6085#pullrequestreview-218105939

This looks so cool. Passing through with a few quick comments:

  • You could (require 'seq) since you use seq-into

  • Some comments on the python (sorry if this is a bit verbose)

    • It seems like the python code exists to create a json data structure, save it as a config file, and then call pywal to reload the theme from that config file. Admittedly it may not be as easy, but couldn't all of this be done in Emacs using the built-in json.el functions? You could potentially remove the Python dependency entirely.
    • That said, there is only a small amount of python code spread across three files (including two "utility" files -- utils.py and pywal_utils.py). If the code will stay small, maybe these should be consolidated into a single file?
    • Moreover, some of the python code looks a little unsafe - running wal --theme "{}" could be an opening for an injection attack; why not use subprocess.call? This doesn't look super risky right now since the "{}" comes from a trusted internal variable, but if the code evolves (speaking from experience) you don't want to get caught by this.