openerp-brazil-team team mailing list archive

[Fabio Negrini <fnegrini@xxxxxxxxx>]

> * 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.

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

> * 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

> * 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!

> 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.
-- 
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.