Adding script (command line) notifier #336
Conversation
Added script settings
Added script options
Added script notifier
Creation of script notifier
Added ScriptConfigurationError
There was a problem hiding this comment.
Hi @sjoerdj,
Thank you for your contribution. I didn't test your code for now, but I want to give you a first review.
- Please use linting and formatting as configured in the pre-commit script. To run it you can type
pre-commit run -a. - Please don't change code, including formatting, that is out of the scope of the feature you want to implement. (
config.sample.iniandconfig.py) - To make your PR perfect, it would be nice, if you provide a unit test for your notifier.
src/notifiers/script.py
Outdated
|
|
||
|
|
||
| def __repr__(self) -> str: | ||
| return " " |
There was a problem hiding this comment.
The returned string is displayed on startup when displaying the activated notifiers.
| return " " | |
| return f"Shell script: {self.command}" |
src/notifiers/script.py
Outdated
| commandline = commandline.replace('${{','~${{') | ||
| commandline = commandline.replace('}}','}}~') |
There was a problem hiding this comment.
What is the reason for this?
There was a problem hiding this comment.
The problem that I faced while testing is that if the unmasked command line item contains spaces (e.g. display_name), the separation of the command line arguments goes wrong. That's why I added a tilde sign before and after the accolades before unmasking.
src/notifiers/script.py
Outdated
| commandlist.append(commandline[start:pos]) | ||
| start = pos+1 | ||
| commandlist.append(commandline[start:pos]) | ||
| commandlist = [a.replace('~','') for a in commandlist] |
There was a problem hiding this comment.
This could make trouble when using a script placed in the home folder and using a path like ~\script.sh.
There was a problem hiding this comment.
I agree that the tilde is not the best choice, a pipe symbol might have been a better idea. Anyway, I now built simpler and much cleaner code to list the arguments, eliminating the need for quoting and unquoting:
commandlist = self.command.split()
commandlist = [item.unmask(a) for a in commandlist]
src/notifiers/script.py
Outdated
|
|
||
|
|
||
| def _send(self, item: Item) -> None: | ||
| import subprocess |
There was a problem hiding this comment.
Why do you place the import here and not on the module level?
There was a problem hiding this comment.
Good one, thanks! I have now put the import at the top of script.py
| self.cron = config.script.get("cron") | ||
|
|
||
| if self.enabled: | ||
| try: |
There was a problem hiding this comment.
Maybe raise a ConfigurationError when the command is missing?
There was a problem hiding this comment.
Good suggestion, thanks! I just added:
if self.enabled and (not self.command):
raise ScriptConfigurationError()
|
Hi @sjoerdj. |
Not at all, please go ahead. Thanks a lot for your review, it has been a nice lesson on clean coding! |
Pull Request Checklist:
make testmake lintmake imagemake executable