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

Add bidirectional call/sms send #18

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

Conversation

walteraa
Copy link

@walteraa walteraa commented Aug 8, 2017

Adiciona a flag --bidirecional para fazer chamada para ambos os números.
gemidao-do-zap --de=x --para=y --token=t --bidirecional

Resolve a issue #16

@haskellcamargo
Copy link
Owner

Parece uma feature legal! Só precisamos adaptar o coding-style:

  • A função bidirecional não tem corpo e não tá sendo usada
  • Rode um npm run lint para ver o que é necessário para adaptar ao coding-style

Obrigado! ❤️

src/cli.js Outdated
@@ -34,6 +51,10 @@ cli(yargs
describe: 'Se definido, será enviado um SMS ao invés de uma chamada',
type: 'boolean'
})
.option('bidirecional', {
describe: 'Defini se a chamada deve ser enviada para ambos os números',

Choose a reason for hiding this comment

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

Sugestão de descrição: Se definido, realiza uma nova ligação, desta vez com o de/para invertidos

Copy link
Owner

Choose a reason for hiding this comment

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

Typo: Defini -> Define

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/cli.js Outdated
var temp = args.de;
args.de = args.para;
args.para = temp;
send(args);

Choose a reason for hiding this comment

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

Sugestão de código utilizando ES6:

send(args);
let { de: para, para: de } = args;
send({...args, de, para});

É importante ressalvar que send() é assíncrono. Eu não sei como a API se comportaria com duas chamadas simultâneas. Talvez fosse bacana você encadear a promise.

Copy link
Owner

Choose a reason for hiding this comment

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

Pela minha configuração do linter atual, até o let vai ser barrado. Ele vai permitir somente const, e vai bloquear reatribuição/mutabilidade (paradigma funcional).

Copy link
Owner

Choose a reason for hiding this comment

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

O ideal também é que a lógica aconteça dentro do gemidao.js, que é onde lida com os argumentos. O cli.js só despacha.

Choose a reason for hiding this comment

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

Opa, bem lembrado, @haskellcamargo. Nem cheguei a ler as configs do Lint ainda.

Copy link
Owner

Choose a reason for hiding this comment

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

Ele tá barrando let, var, try, for e while, pelo que lembro.

@haskellcamargo
Copy link
Owner

Vou testar a branch na prática a branch ao chegar em casa para poder mesclar!
❤️ Obrigado!

@walteraa
Copy link
Author

walteraa commented Aug 8, 2017

@haskellcamargo quando eu chegar em casa vou dar uma melhorada, passar a usar promises. Mas se quiser ir aceitando o PR já pode, posso abrir outro em breve.

Copy link
Collaborator

@williamokano williamokano left a comment

Choose a reason for hiding this comment

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

:)

src/cli.js Outdated
if (args.bidirecional) {
send(args);
const temp = args.de;
args.de = args.para;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Não querendo ser muito chato, mas já sendo, esse pedacinho que vc faz o swap, da pra dar uma melhoradinha, não? rsrs

Copy link
Collaborator

@cuchi cuchi Aug 9, 2017

Choose a reason for hiding this comment

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

Eu usaria o ramda:

const swapValues = (key1, key2, obj) => pipe(
  assoc(key1, obj[key2]),
  assoc(key2, obj[key1]))
    (obj);

// ...
if (args.bidirecional) {
  send(args);
  send(swapValues('de', 'para', args))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Achei elegante

Copy link
Author

@walteraa walteraa Aug 9, 2017

Choose a reason for hiding this comment

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

Resolvi desacoplando os dados da msg, isso possibilita modificar o padrão from/to e fazer outra chamada send após gerar a call invertendo os valores, o que vocês acham @williamokano @cuchi?

src/cli.js Outdated
if (args.bidirecional) {
send(args);
const temp = args.de;
args.de = args.para;
Copy link
Collaborator

@cuchi cuchi Aug 9, 2017

Choose a reason for hiding this comment

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

Eu usaria o ramda:

const swapValues = (key1, key2, obj) => pipe(
  assoc(key1, obj[key2]),
  assoc(key2, obj[key1]))
    (obj);

// ...
if (args.bidirecional) {
  send(args);
  send(swapValues('de', 'para', args))

@walteraa walteraa changed the title Add bidirecional call/sms send Add bidirectional call/sms send Aug 9, 2017
Copy link
Collaborator

@williamokano williamokano left a comment

Choose a reason for hiding this comment

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

PT BR?

.send(msg_data(from, to));

function msg_data(from, to) {
return {
numero_destino: to,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deixa as variáveis em inglês. E bina é o sistema de identificação, não sei se seria o melhor nome para deixar o from, prefiro os nomes originais from e to

Copy link
Author

Choose a reason for hiding this comment

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

@williamokano eu mantive o que vinha sendo feito porque isso é a estrutura de dados que o endpoint recebe, os nomes dos parâmetros foram mantidos também, só fiz desacoplar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@walteraa numero_destino e bina já existiam?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, viajei aqui, talvez sejam requisitos para enviar a totalvoice.

Copy link
Author

Choose a reason for hiding this comment

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

Sim, são os dados recebidos pelo totalvoice, segundo a doc deles(https://api2.totalvoice.com.br/doc/#!/Composto/post_composto)

@cuchi
Copy link
Collaborator

cuchi commented Aug 9, 2017

Não gostou da minha ideia? 😢

@walteraa
Copy link
Author

walteraa commented Aug 9, 2017

Sim @cuchi, mas desacoplar os dados e usar promises "seems better"!

@AndreyMenezes
Copy link

Puts @walteraa !

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.

6 participants