refactor: Melhorias nas funções de validação de CNPJ#754
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #754 +/- ##
==========================================
- Coverage 99.09% 99.08% -0.01%
==========================================
Files 26 26
Lines 775 767 -8
==========================================
- Hits 768 760 -8
Misses 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
its-Sohan
left a comment
its-Sohan
left a comment
There was a problem hiding this comment.
overall a nice refactoring. the regex centralization makes the format validation much cleaner than the scattered manual checks. a few observations inline.
its-Sohan
left a comment
its-Sohan
left a comment
There was a problem hiding this comment.
overall a nice refactoring. the regex centralization makes the format validation much cleaner than the scattered manual checks. a few observations inline.
its-Sohan
left a comment
its-Sohan
left a comment
There was a problem hiding this comment.
overall a nice refactoring. the regex centralization makes the format validation much cleaner than the scattered manual checks. a few observations inline.
| @@ -1,7 +1,10 @@ | |||
| import random | |||
| import re | |||
| from itertools import chain | |||
There was a problem hiding this comment.
good use of a compiled regex constant. centralizing the format pattern is much cleaner than the repeated manual length + char checks in display() and validate().
| from string import ascii_uppercase, digits | ||
|
|
||
| CNPJ_RE = re.compile(r"^[A-Z0-9]{12}[0-9]{2}") | ||
|
|
There was a problem hiding this comment.
nice simplification. the filter(lambda) pattern was harder to read. re.sub with a character class is both faster and clearer.
| @@ -31,7 +34,7 @@ def sieve(dirty: str) -> str: | |||
| backward compatibility. | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
this removes the len(set(cnpj)) == 1 check, so 00000000000000 now formats as 00.000.000/0000-00 instead of returning None. you mentioned this in the pr description, which is good. just noting it's a behavioral change for callers that relied on display() rejecting all-same-character inputs.
| return re.sub(r"[\.\/\-]", "", dirty) | ||
|
|
||
|
|
||
| def remove_symbols(dirty: str) -> str: |
There was a problem hiding this comment.
nice - removing _is_alphanumeric is the right call since the regex now handles that check. less code to maintain.
| if ( | ||
| len(cnpj) != 14 | ||
| or not _is_alphanumeric(cnpj[:12]) | ||
| or not cnpj[12:].isdigit() |
There was a problem hiding this comment.
one concern here: random.sample() picks without replacement, so the 8-character base will never have repeated characters. random.choices() (which was used before) allows repeats. real cnpjs can have repeated digits (e.g. 11.111.111/0001-11), so sample reduces the space of valid generated cnpjs. consider whether choices was more correct here, or if you want repeats in the generated base.
its-Sohan
left a comment
its-Sohan
left a comment
There was a problem hiding this comment.
desculpe, escrevi a review em ingles sem perceber que o projeto é brasileiro. aqui vai a versão em portugues:
no geral, uma boa refatoração. centralizar a validação de formato com a regex CNPJ_RE ficou muito mais limpo do que as verificações manuais espalhadas.
pontos principais:
re.subnosieve()é mais simples quefilter(lambda)- boa simplificação- remover o
_is_alphanumericfoi a decisão certa já que a regex cobre isso agora - o
random.sample()vsrandom.choices():sampleescolhe sem repetição, então os 8 caracteres da base nunca vão ter dígitos repetidos. cpfs reais podem ter repetidos (ex:11.111.111/0001-11), entãosamplereduz o espaço de cnPJs válidos gerados. vale reconsiderar usarchoicespra permitir repetição. - a mudança no
display("00000000000000")que agora retorna00.000.000/0000-00ao invés denone- voce mencionou na descrição, mas é uma mudança de comportamento que pode afetar quem usava o retornonone.
no mais, bom trabalho!
2 participants
Descrição
Este PR simplifica o código para validação e geração de CNPJs.
Mudanças Propostas
randomChecklist de Revisão
Comentários Adicionais (opcional)
display) por não ser um CNPJ válido. Para manter a função mais simples, eu removi essa restrição, assimCNPJ_REpode ser utilizado nela sem problemas. Dado que é apenas uma função que exibe informação na tela, não considerei algo crítico para o sistema, já que o input é fornecido pela pessoa utilizando a biblioteca.Issue Relacionada
Não há uma issue. É apenas uma refatoração de código que já está funcionando.
Closes #<numero_da_issue>