Add hook plugin (fixes #1561) #1603
Conversation
This plugin allows users to execute scripts on different events, as well as forward any arguments from the events to the script.
Iterate substitute_args instead of kwargs, as we ignore anything that is not in substitute_args already. Fix an issue where a hook argument containing non-ascii characters caused an exception.
|
|
||
| # TODO: Find a better way of converting arguments to strings, as I | ||
| # currently have a feeling that forcing everything to utf-8 might | ||
| # end up causing a mess. |
There was a problem hiding this comment.
Indeed; we'll need to encode the strings as bytes using the platform's preferred encoding. That's the same encoding we use for decoding arguments, from the _arg_encoding function.
|
Oops! Somehow didn't see |
|
No worries, feel free to take over whatever you possibly need from |
| if key in kwargs: | ||
| hook_command = hook_command.replace(substitute_args[key], | ||
| unicode(kwargs[key], | ||
| _arg_encoding())) |
There was a problem hiding this comment.
Tiny detail: I usually think s.decode(encoding) is more logical than unicode(s, encoding).
|
Sorry for falling down on this review here! This looks like it's in great shape—nearly ready to merge. |
|
Really like this plugin! Is there a way to use the album path on event Tried with: And: Both end up with: |
- Remove `shell` option and split all commands using `shlex.split` before passing them to `subprocess.Popen`. - General refactor of hook plugin code - move hook creation function inside `HookPlugin`. - Add improved error handling for invalid (i.e. empty) commands or commands that do not exist.
- Fix `test_event_X` name collision between tests causing tests to fail unexpectedly. - Update tests to match new hook plugin design (i.e. remove shell and subtitution option testing).
|
@kooimens, you should be able to do this now (since 8b4f349): hook:
hooks:
- event: album_imported
command: echo "writing to \"{album.path}\"" |
|
Good point about exception handling. I think the right thing to do here may be to just print an error message and move on. That is, beets should complain loudly but not actually stop execution if the command execution fails, whether it's because the command can't be found or because it returns a nonzero exit status. That would look something like this: In general, I use these rules for deciding how to handle errors:
|
|
Sounds good, I'll update the code to reflect that. |
Current coverage is 76.86%@@ master #1603 diff @@
==========================================
Files 62 63 +1
Lines 12900 12930 +30
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 9912 9938 +26
- Misses 2988 2992 +4
Partials 0 0
|
|
Thank you for seeing this through to the conclusion! I'm really happy with how it turned out. I just merged the new plugin with a few rewordings in the docs. |
|
Lovely, thanks! 😄 |
|
This is awesome. Great job, @jackwilsdon! |
Adds a hook plugin that allows commands to be run when a beets event is emitted, as well as tests and documentation for the plugin. This PR fixes #1561.
There's some sections of this code that I'm not all too comfortable with, mainly how hook_function implements STDOUT redirection (feels a bit hacky, however I can't see a better way at the moment), as well as how hook_function handles stringifying arguments (will this fail under different encodings?).
Also, is there a better way of doing what I'm doing here? I'm having to call
load_pluginafter generating the configuration, which just feels wrong to me. This is due to how the plugin itself is coded, as all of the hooks are created inside__init__, which is called whenload_pluginexecutes (so I can't put theload_plugincall inside thesetUpmethod). Would moving the initialisation code to a listener forpluginloadfix this? (although then we'd need to call allpluginloadhooks in the configuration manually, as we're only adding them afterpluginloadhas already been emitted by beets)Looking for feedback on the overall code quality.