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

Remover Símbolos do Número de Telefone #188

Merged
merged 16 commits into from
Oct 4, 2023

Conversation

giovannism20
Copy link
Contributor

@giovannism20 giovannism20 commented Oct 2, 2023

Descrição

#187

Mudanças Propostas

Checklist de Revisão

  • Os testes foram adicionados ou atualizados para refletir as mudanças (se aplicável).
  • Foi adicionada uma entrada no changelog / Meu PR não necessita de uma nova entrada no changelog.
  • A documentação em português foi atualizada ou criada, se necessário.
  • Se feita a documentação, a atualização do arquivo em inglês.
  • Eu documentei as minhas mudanças no código, adicionando docstrings e comentários. Instruções
  • O código segue as diretrizes de estilo e padrões de codificação do projeto.
  • Todos os testes passam. Instruções
  • O Pull Request foi testado localmente. Instruções
  • Não há conflitos de mesclagem.

Comentários Adicionais (opcional)

Issue Relacionada

Closes #187

@giovannism20 giovannism20 requested review from a team as code owners October 2, 2023 19:19
@giovannism20 giovannism20 marked this pull request as draft October 2, 2023 19:22
CHANGELOG.md Outdated Show resolved Hide resolved
brutils/phone.py Outdated Show resolved Hide resolved
@giovannism20 giovannism20 marked this pull request as ready for review October 2, 2023 19:35
@giovannism20
Copy link
Contributor Author

Fiquei com algumas dúvidas e coloquei nos comentários mesmo como review, conforme forem respondendo, atualizo mais o pull request!

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #188 (b38059a) into main (b9c8bd1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #188   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines           90        93    +3     
=========================================
+ Hits            90        93    +3     
Files Coverage Δ
brutils/phone.py 100.00% <100.00%> (ø)

Copy link
Member

@camilamaia camilamaia left a 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!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
documentation v1.0.1/ENGLISH_VERSION.md Outdated Show resolved Hide resolved
tests/test_phone.py Show resolved Hide resolved
@giovannism20
Copy link
Contributor Author

@camilamaia Atualizei os locais corretos das documentações agora e resolvi os conflitos. Se tiver mais algum ponto, só pontuar que modifico por aqui ;)

README_EN.md Outdated Show resolved Hide resolved

```python
>>> from brutils import remove_symbols_phone
>>> remove_symbols_phone('+55 (21) 2569-6969')
Copy link
Member

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.

Copy link
Contributor Author

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 :)

Copy link
Member

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

@camilamaia
Copy link
Member

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!"
Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

@camilamaia camilamaia left a 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!

@antoniamaia antoniamaia merged commit c0044a5 into brazilian-utils:main Oct 4, 2023
8 checks passed
@antoniamaia antoniamaia mentioned this pull request Jan 5, 2024
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.

Remover Símbolos do Número de Telefone
3 participants