Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIG] Movido de __unported__ y PEP8 #31

Merged
merged 4 commits into from
Jul 22, 2014

Conversation

rlizana
Copy link
Contributor

@rlizana rlizana commented Jul 22, 2014

Comprobada la funcionalidad del modulo y OK

@rlizana
Copy link
Contributor Author

rlizana commented Jul 22, 2014

No estoy de acuerdo con la regla PEP8 E501 (line too long), en el 2001 donde se tenían monitores de 15" no panorámicos tendría sentido, pero a día de hoy creo que esta desfasada. En el código de Odoo no se aplica esta regla de estilo... ¿se podría omitir?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.46%) when pulling 1e00042 on rlizana:v8_l10n_es_account_asset into 78c42a9 on OCA:8.0.

@pedrobaeza
Copy link
Member

Roberto, sobre lo de las líneas demasiado largas es algo que aplica el PEP8, y que aunque como bien dices los monitores de hoy día son más largos, tienes varios casos en los que es bastante útil:

  • Comparación "side to side" (cualquier herramienta de diff) de dos archivos.
  • Conexiones SSH con editores de comando tipo nano, vi, etc.
  • Edición en un IDE con múltiples ventanas.

Al principio, a mí también me resultó extraño hacerlo, pero una vez te acostumbras, ya te resulta natural y útil, porque a la larga, verás como es más cómodo que tus ojos no tengan que hacer tanto "scroll lateral" aunque tengas un monitor 4K 😉

De todas formas, en los tests está puesto el PEP8 ampliado, para que las líneas puedan ser de hasta 120 cols, no 80, dando un margen intermedio.

Sobre el MP, ¿podrías por favor cambiar el icono a la nueva ruta para que lo coja desde la gestión de módulos? También, Travis da un falso positivo, porque muestra un error de que la columna ext_method_time no puede contener un valor null. Eso es debido a que no tiene un valor por defecto. Desde OCA ya se está mirando para que cante. ¿Podrías mirar de arreglarlo?

Gracias.

Un saludo.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.46%) when pulling fdcc4e8 on rlizana:v8_l10n_es_account_asset into 78c42a9 on OCA:8.0.

@rlizana
Copy link
Contributor Author

rlizana commented Jul 22, 2014

Arreglado ;)

@pedrobaeza
Copy link
Member

Muchas gracias por los cambios!

👍

pedrobaeza added a commit that referenced this pull request Jul 22, 2014
[MIG] l10n_es_account_asset: Movido de __unported__ y PEP8
@pedrobaeza pedrobaeza merged commit 839a7af into OCA:8.0 Jul 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants