Conversation
This allows folks to set custom per-feed recipients via the "notify" option, which will replace the recipients set in the cron command arguments. Fixed: skx#73
|
Looks good to me, but do not forget to update the documentation, i.e. README.md |
|
I think we don't need any more changes to the docs, since there is a pointer to run Lines 81 to 90 in 43cb238 |
|
A quick glance looks good, but I'm not 100% awake having just returned from a trip/holiday. Only thing that jumps out is that you changed the comment(s) referring to "cron command", but there's also the "daemon command" (which I use personally, as I run this software inside a docker container. My suspicion would be that both would do the expected thing, because your real change is in the shared |
|
Done! Yup, in theory both commands should behave in the same way, so this was indeed a mistake. It should be fixed in the last commit. btw, I also run rss2email via Docker with the daemon command, so I wonder why I forgot about its existence :P Thanks for catching that, I completely messed up! |
|
Hmmm, I don't remember how GitHub works like when merging PRs (I usually use Gerrit), but I've just changed the main message in this PR in case it's used as the commit message when/if the PR gets merged. |
|
You didn't mess up at all, you made a useful change in the code and I appreciate all contributions be they big or small. It's fun to see other people using this tool, and it's awesome to see people take time out of their busy lives to make changes to it. Looks good to me. Will do a bit of a manual test later today and merge after that checks out! |
This allows folks to set custom per-feed recipients via the "notify"
option, which if set will be used instead of the recipients set in the
cron or daemon command arguments.
Fixed: #73