Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

Add SMS sending #78

Merged
merged 2 commits into from
Jul 22, 2016
Merged

Add SMS sending #78

merged 2 commits into from
Jul 22, 2016

Conversation

cbetta
Copy link
Contributor

@cbetta cbetta commented Jul 7, 2016

Adds sending of an SMS. Only implements the basics to keep things simple. It auto detects if a from sender has been given and if not uses Nexmo CLI as the from field.

> nexmo sms <destination_number> "Hello world!" --confirm
Message sent to:   <destination_number>
Remaining balance: 26.80110000 EUR
Message price:     0.03330000 EUR

> nexmo sms <from_name_or_number> <destination_number> "Hello world!" --confirm
Message sent to:   <destination_number>
Remaining balance: 26.80110000 EUR
Message price:     0.03330000 EUR

Closes #17


// sending messages

sendSms(first, second, third, command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why this variable naming convention and not calling them to, from etc. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause it's not to, from etc. Depending on the number of variables provided it has a different meaning, see line 234 and 236

Copy link
Contributor

Choose a reason for hiding this comment

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

When the number of arguments isn't set you normally see arguments used, so I found that a bit confusing.

In ES2015 there's ... which is used for the "Rest of the arguments". See https://babeljs.io/docs/learn-es2015/#default--rest--spread

The parameter names - to me - are a little unclear right now. But this is just a nitpick.

@cbetta
Copy link
Contributor Author

cbetta commented Jul 11, 2016

Based on feedback from @leggetter the syntax has changed to:

> nexmo sms <destination_number> Hello world! --confirm
Message sent to:   <destination_number>
Remaining balance: 26.80110000 EUR
Message price:     0.03330000 EUR

> nexmo sms  <destination_number> Hello world! --from "Acme Inc" --confirm
Message sent to:   <destination_number>
Remaining balance: 26.80110000 EUR
Message price:     0.03330000 EUR

This is mostly because the previous syntax was very error prone. CommanderJS doesn't really like variable arguments so it's better to just override the from with a flag.

I also made sure that the text field is now a variadic field, allowing us to capture Hello World without surrounding it in ".

@leggetter leggetter merged commit 2e0731e into master Jul 22, 2016
@leggetter leggetter deleted the 17-send-sms branch July 22, 2016 11:44
@sammachin
Copy link
Contributor

Alphanumeric Senders don't support spaces anyway, just testing on the API with the python lib whatspace is removed from the sender somewhere along the line Sam Machin became SamMachin at the handset

@cbetta cbetta restored the 17-send-sms branch July 22, 2016 14:08
@cbetta cbetta deleted the 17-send-sms branch July 22, 2016 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants