openerp-brazil-team team mailing list archive

["Leonardo Santagada @ Proge.com.br" <santagada@xxxxxxxxxxxx>]

Em 4 de novembro de 2011 14:39, Fabio Negrini <fnegrini@xxxxxxxxx> escreveu:
>> * text_only_number deve ser renomeada para "only_number" ser uma função e
>> definida como algo:
>>
>> delete_chars = string.punctuation + ' '
>> def only_number(s):
>>   return str(s).translate(None, delete_chars)
> Sugestão aceita, porém mudei o nome para "string_only_number", acredito que este nome denota bem sua funcionalidade
>
>>
>> * text_normalize deve ser uma função tb e pode se chamar normalize.
> Seguindo a mesma idéia acima, troquei o nome para "string_normalize", além do que já existe uma função normalize e acho melhor não usar o mesmo nome.
>
otimo se bem que number_only_string e normalize_string é mais correto em inglês.

>> * Remover os or '', eles são necessários?
> Acho que códigos redundantes são sempre bem-vindos

Não, são sempre mal vindos, eles escondem bugs que não deveriam passar
despercebidos.

>
>> * invoice_access_key poderia usar uma só interpolação de string para fazer
>> todas as justificações e fazer strip das strings.
> Mantive em várias etapas por ficar mais fácil a sua depuração

Não ajuda em nada a depuração, só faz o código ficar mais longo e repetitivo.

>> * quem sabe ao invés de centenas de linhas duplas usar uma função:
>>
>> def subelement_text(root, name, text):
>>   t = SubElement(root,name)
>>   t.text = text
>>   return t
> Sugestão aceita! Implementado!
>
otimo

>> Eu sei que isso seria mais um refactoring, mas faria o código ficar tão mais
>> limpo que valeria a pena.
>>
>> * usar enumerate no for ao invés de manter um indice manual
>>
>> * remover linhas em branco desnecessárias, como 414, 469
>>
>> * Usar a ferramenta pep8 para verificar a formatação do código e a pyflakes
>> para mostrar variaveis desnecessárias
>>
>> * linha 474: nfeProc.extend([NFe]) para nfeProc.append(NFe)
>>
>> Isso foi só um review rápido eu devo ter perdido vários detalhes a mais, mas
>> fazendo as revisões propostas acima acho que os outros detalhes vão ficar mais
>> visiveis.
>
> Minha proposta é mexer somente no código da geração do XML, não vi nem mexi em mais nada além disso. Não vou mexer nisso agora... talvez quando estiver mais tranquilo.

no teu diff tu tocou nessas linhas, então sim tu tocou nisso. Não é um
grande problema, mas se a ideia é que o código fique progressivamente
melhor é sempre bom fazer commits que melhoram o código se tu precisa
mexer nele.

-- 
https://code.launchpad.net/~fnegrini/openerp.pt-br-localiz/openerp.pt-br-localiz/+merge/80941
Your team OpenERP Brazil Team is subscribed to branch lp:openerp.pt-br-localiz.