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

Create WhatsApp_sender.py #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gautam1858
Copy link

Adding whatsapp notification

Adding whatsapp
@VictorSanh
Copy link
Contributor

Hello @gautam1858,
I might be missing something: why is the whatsapp_sender file exactly the same as sms_sender.py? If it's the same, then using the same call should be sufficient (I haven't checked how to send messages through WhatsApp through python).
Also, please test and check your code for obvious errors (for instance, the file whatsapp_sender should have a .py extension). I also prefer having all the changes in the same pull request (cf PR #33 ).
Victor

@gautam1858
Copy link
Author

gautam1858 commented Jan 8, 2020

@VictorSanh Have made the necessary changes that you have asked for, its similar to sms but here I have added prefix WhatsApp so that its compatible for WhatsApp 🤗

@VictorSanh
Copy link
Contributor

Ok, I understand how it works now.

I would have a more simple/direct approach:

def whatsapp_sender(account_sid: str, auth_token: str, recipient_number: str, sender_number: str):
    prefix = 'whatsapp:'
    return sms_sender(account_sid=account_sid, recipient_number=prefix+recipient_number, sender_number=prefix+sender_number)

The code is exactly the same modulo this prefix, so no need to duplicate a whole bunch of code (plus you have to make sure all the cases+exceptions are covered).

Also, before merging, you also need to make the modifcations to the README, __init__.py and __main__.py. For instance, you can have a look at this complete pull request (#36) I merged a few days ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants