PR Checklist

⚠️ THIS IS A DRAFT!

https://dev.to/mporam/good-code-reviews-43kk

https://medium.freecodecamp.org/why-i-changed-the-way-i-think-about-code-quality-88c5d8d57e68

• Revisa con rubocop los ficheros que has tocado (mejor si es sólo las lineas que has modificado

• Revisa el Diff total, verás cosas que escribiendo codigo no nos damos cuenta

• Revisa el issue de nuevo ¿completas todos los requisitos?

• Algun drawback importante que pueda tener un fork si se mergea esto?

• Tests… hemos cubierto todos los escenarios posibles (positivos/negativos)? hemos roto algunos otros en el proceso (travis nos ayuda en esto)?

• Titulo del PR... Hay que rellenarlo, no vale el nombre autocompletado por github con el nombre de la rama! Debe ser descriptivo, como para entrar en el Changelog y ser entendible por cualquiera que esta ocurriendo.

• Description del PR… ¿puede alguien con leerse el issue y el description comprender como se ha solucionado el problema?

• Commits ¿explican lo que se esta cambiando y porque? ¿podríamos escribir el PR description sólo mencionando commit hashes? (un ejemplo de PR haciendo referencias a commit hashes https://github.com/consul/consul/pull/2285) Usa git rebase si es necesario, o un simple git reset master y volver a recrear la historia con commits https://help.github.com/articles/about-git-rebase/

• Si estas haciendo un Refactor necesario para acomodar las nuevas funcionalidades o cambios, que podría ser entendido de forma independiente... es preferible hacer una PR previa con ese refactor:

  • Cuando alguien quiera revisar en el futuro los cambios importantes (el objetivo de la PR actual) no va a tener que leer/entender el refactors… si no esta relacionado (podría haber sido hecho hace meses) no debería estar asociado.
  • Divide en dos la carga de revisión (los objetivos a los que hay que prestar atención y evaluar si se cumplen)

• ¿Hemos explorado otras alternativas de implementación? ¿porque las hemos descartado? A veces no comentar esto conlleva comentarios en el PR preguntando por ello… podemos adelantarnos. Otras conlleva falta de confianza en que se ha dado suficiente tiempo a pensar en la solución… la confianza es clave en mayusculas a la hora de aceptar PR’s por desgracia.

• Obviamente rellenar el PR template (esta ahi como guia para ayudarte a recordar todas las preguntas que tiene un Reviewer) y tambien el Title (no autorellenar con el texto del primer commit ni tampoco con el nombre del autor!)

• Incluyes una migracion que borra columnas o tablas de la bbdd? Has planeado hacer correctamente la deprecación de esos elementos previamente? https://github.com/AyuntamientoMadrid/consul/wiki/How-to-do-Deprecations-(When-removing-database-columns)

• Incluyes algun valor en settings nuevo? Deben aparecer también en seeds.rb y dev_seeds.rb. Crea una rake para darle un valor inicial si es necesario.

• Deberíamos crear/alterar seeds.rb y/o dev_seeds.rb para poder probar esta funcionalidad/cambio en local?

• Si estas usando Date/Time/DateTime asegurate de que usas correctamente la zona horaria. En vez de Date.new(... usa Time.zone.local(... con un .to_date al final si es necesario usar fechas (así evitamos problemas con cambios de TimeZone)

• Has revisado las Coding guides? https://github.com/AyuntamientoMadrid/consul/wiki/Coding-Guides (en desarrollo)