avanzosc team mailing list archive

[Pedro Manuel Baeza <pedro.baeza@xxxxxxxxx>]

Review: Needs Fixing

Buenas, Oihane,

Gracias por la contribución. Realizando una revisión de código, hay varias cosas que se podrían hacer para mejorarlo (te indico línea del diff como referencia):

- l.74: la comparación de las cantidades transformándolas a cadena no es el mejor método y además puede dar falsos negativos. Aunque ya sé que ese código ya estaba, puedes aprovechar para mejorarlo cogiendo una instancia de res.currency para la moneda de la compañía y llamar al método "is_zero" (o alguien así, que hablo de memoria), restando ambas cantidades, para que te devuelva correctamente la comparación.

- l.85 y l.89: Puesto que en ambos objetos se escriben los mismos datos, lo que puedes hacer para simplificar código y que sea más legible es lo siguiente:

    if invoice.type=="out_invoice" or invoice.type=="out_refund":
        obj = invoices340
    else:
        obj = invoices340_rec
    obj.write(cr, uid, invoice_created,
              {'amount_tax': tot_tax_invoice,
               'warning': warning,
               'warn_message': error})

- En todo el código: por favor, intenta aplicar PEP8 en el código que modifiques/insertes. De esta forma, se homogeneiza el estilo de programación en todo el repositorio. Puedes consultar aquí las reglas de este estilo: http://legacy.python.org/dev/peps/pep-0008/

- l.98-100, l.106-109: En lugar de comentar esas líneas, mejor eliminarlas directamente, ya que así en el diff se lee mucho mejor y no queda código basura.

Por último, debido al cambio de orientación, debería ampliarse la funcionalidad con las siguientes características:

- Avisar al terminar el cálculo de que hay algún error en las líneas.
- Comprobar antes de confirmar la declaración si queda alguna línea con warning, ya que de otro modo, alguien podría seguir adelante con la declaración y emitirla errónea.
- Permitir eliminar a mano el warning.

Un saludo.

Un saludo.
-- 
https://code.launchpad.net/~avanzosc/openerp-spain/6.1/+merge/211521
Your team Avanzosc Developers is subscribed to branch lp:~avanzosc/openerp-spain/6.1.