Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix email notifications for shows with special cars. #9652

Merged
merged 4 commits into from
Jun 19, 2021

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Jun 19, 2021

  • Make sure to look up the show by id's, in stead of by name.
  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

fix #9500

* Make sure to look up the show by id's, in stead of by name.
@@ -65,15 +65,15 @@ def test_notify(self, host, port, smtp_from, use_tls, user, pwd, to):
msg['Date'] = formatdate(localtime=True)
return self._sendmail(host, port, smtp_from, use_tls, user, pwd, [to], msg, True)

def notify_snatch(self, title, message):
def notify_snatch(self, title, message, ep_obj):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to make it a keyword argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda did this on purpose. I had it as keyword first. But it's always called with an ep_obj. So no need to make it default to None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't this defeat the purpose to use **kwargs for all other notifiers? We are forced to always add the email argument for all notifiers like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the **kwargs param just prevents them from Errorring. As it's now its not causing any errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that it is pretty confusing right now. The method header should be equal for all and the arguments should be handled inside the function IMO.

* Update docstrings
@p0psicles p0psicles merged commit c41ead3 into develop Jun 19, 2021
@p0psicles p0psicles deleted the feature/fix-email-chars branch June 19, 2021 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants