-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Remover Símbolos do Número de Telefone #188
Conversation
Fiquei com algumas dúvidas e coloquei nos comentários mesmo como review, conforme forem respondendo, atualizo mais o pull request! |
Codecov Report
@@ Coverage Diff @@
## main #188 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 90 93 +3
=========================================
+ Hits 90 93 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muito obrigada pela contribuição!! Deixei alguns comentários com sugestões de mudanças. Também atualizamos nossa doc para deixar mais clara as instruções para as próximas.
Valeu demais!
@camilamaia Atualizei os locais corretos das documentações agora e resolvi os conflitos. Se tiver mais algum ponto, só pontuar que modifico por aqui ;) |
|
||
```python | ||
>>> from brutils import remove_symbols_phone | ||
>>> remove_symbols_phone('+55 (21) 2569-6969') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pelo o quê eu entendi da tua implementação, ela não estaria tirando o +
nem o
(espaço), certo? Então aqui o resultado seria +55 21 25696969
ao invés de 552125696969
se entendi certo,
Pensando aqui, acho que seria legal tirar esses dois símbolos também. @antoniamaia o quê você acha? Daí na issue de retirar o código do país, teria que passar a string sem símbolo nenhum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atualizei a implementação e adicionei mais casos de teste :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamaia Sim!! faz todo sentido
Valeu @giovannism20 ! Muito obrigada. Fiquei com uma dúvida só sobre quais símbolos remover, deixei ali comentado no código :) No mais, tá show!! |
|
||
# When there are extra symbols, it only removes the specified symbols | ||
self.assertEqual( | ||
remove_symbols_phone("(21) 99402-9275!"), "21994029275!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acham que faria sentido removermos ainda outros símbolos e realmente deixar apenas números ?
cc: @camilamaia @antoniamaia
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giovannism20 o padrão que estamos seguindo para os outros utilitários é de remover apenas os símbolos de cada contexto.. até poderia fazer sentido, mas como estamos seguindo este padrão poderia causar confusão para quem já utiliza a biblioteca.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aeee, valeu demais, agora tá 10/10!
Descrição
#187
Mudanças Propostas
Checklist de Revisão
Comentários Adicionais (opcional)
Issue Relacionada
Closes #187