Fix Package diameter property #118
Conversation
tests/test_posting_models.py
Outdated
| ('TYPE_BOX', 0), | ||
| ('TYPE_CYLINDER', 16), | ||
| ]) | ||
| def test_package_diameter(package, package_type, diameter): |
There was a problem hiding this comment.
I think you should add a paremeter with diameter larger than MIN_DIAMETER to test the second condition of max() call.
|
done @cacarrara |
correios/models/posting.py
Outdated
| @@ -331,7 +331,9 @@ def length(self, length): | |||
|
|
|||
| @property | |||
| def diameter(self) -> int: | |||
| return max(MIN_DIAMETER, int(math.ceil(self.real_diameter))) | |||
| if self.package_type == Package.TYPE_CYLINDER: | |||
There was a problem hiding this comment.
inverte a lógica... trate a exceção primeiro e retorne o valor "default" na sequencia:
if self.package_type != Package.TYPE_CYLINDER:
return 0.0
return max(...)There was a problem hiding this comment.
Agora com mais tempo pra explicar:
Em um método que deve retornar um valor calculado "exceto em certos casos" fica mais elegante tratar as "exceções" primeiro para que elas fiquem evidentes no corpo do método.
E se o programador está lendo o método que retorna um diâmetro mas se interessa só pelo retorno "default" os olhos podem ir direto para o fim do método onde ele vai encontrar um comando "return" que retorna o valor que ele quer na maior parte das vezes...
Entenderam o racional? O objetivo dessa mudança é deixar o método (property nesse caso) fácil de ler para quem vai usar ela e fácil de entender para quem vai refatorar/reescrever ela...
tests/test_posting_models.py
Outdated
| ('TYPE_CYLINDER', 18, 18), | ||
| ]) | ||
| def test_package_diameter(package, package_type, diameter, result): | ||
| package.package_type = getattr(package, package_type) |
There was a problem hiding this comment.
parametrize direto com Package.TYPE_ENVELOPE e tira esse getattr()
There was a problem hiding this comment.
Pergunta: já não tinha testes para dimensões de pacotes cilindricos? não dava pra adicionar isso lá?
There was a problem hiding this comment.
Eu criei um novo teste para abordar o diâmetro de todos os tipo de objetos, e não somente do tipo cilindrico.
There was a problem hiding this comment.
beleza... então refatora só pra tirar aquele getattr ali..
There was a problem hiding this comment.
Com mais calma: explícito é melhor do que implícito. 🙂
Quando escrevemos um teste é importante que ele também possa servir como algo próximo de um "documento de uso".
Quando colocamos uma string no parametrize desse teste pode passar a mensagem para a pessoa que não viu a implementação do teste que o parâmetro que passamos para o método é do tipo string, o que não é verdade. Mais uma vez é uma questão de escrever código que transmita informação para quem quer ler ele rapidamente e já saber como usar...
There was a problem hiding this comment.
Nice 👍
In [1]: import this
The Zen of Python, by Tim Peters
Explicit is better than implicit.
|
assim que passar no CI eu já faço merge... posso fazer release na sequencia? |
|
Yes, you can :) |
According to new version of Correios' documentation, the package diameter only needs to be calculate if the object type is equal 003, cylinder, otherwise must return 0, as we can see in the image below.