El modelo Pull Request ha resultado ser una excelente forma de construir software en equipos, especialmente para equipos distribuidos. No sólo para el desarrollo de código abierto, sino también en las empresas.

En la mayoría de los casos, hay uno o más revisores involucrados, que tendrán que revisar una misma pull request. La tarea del desarrollador ya no es sólo escribir bien el código, ahora también hay que atender a la/s persona/s que están revisando tus pull requests o revisar las del resto del equipo.

La revisión de código es una parte muy importante del ciclo de desarrollo de software. También es una de las partes más complejas y que consumen más tiempo en el proceso de desarrollo, a menudo, requiere que los miembros experimentados del equipo pasen tiempo leyendo, pensando, evaluando y respondiendo a implementaciones de nuevas características del sistema.

Las pull request (en adelante PRs) aportan una cantidad de bondades que van más allá del incremento de la calidad del código. Entre otras cosas aportan poder aprender unos miembros de otros, contexto en el trabajo que realizan nuestros compañeros, homogeneización del estilo del código, unificación de procesos y construir un equipo sólido (Scrum).

Por lo tanto que hacer una buena pull request implica algo más que escribir buen código.

Revisar las pull requests es difícil

Admitámoslo: revisar una “Pull Request” (PRs) es complicado.

Como revisor, tu responsabilidad es asegurarse de que el código es correcto y de calidad antes de que sea integrado en la rama master. Se espera que lo hagas mirando un diff y una lista de archivos cambiados. Tienes que entender que trata de lograr el PR, qué enfoque se está adoptando, qué está pasando y cómo todos los archivos encajan… todo esto para sugerir una mejora.

También hay que buscar errores tipográficos o problemas de estilo. Es una tarea extensa, sobre todo en una PR muy larga.

¿Cómo podemos hacer la tarea más fácil para nuestros compañeros? Solo tenemos que ponernos en la piel del revisor y preguntarnos, «¿Cómo podría hacer esto más fácil de revisar?»

Hacer pull requests pequeñas

Todo el mundo coincide en esto. Hacer PRs más pequeñas es lo más efectivo a la hora de acelerar el tiempo de revisión. Al ser más pequeñas, es más fácil para los revisores entender el contexto y la razón de la PR.

Se necesita tiempo para revisar adecuadamente una PR y el tiempo que se necesita es exponencial al tamaño; La relación no es lineal.

Es difícil hacer PRs pequeñas, lógicas y rápidas de revisar. Esto conlleva práctica, una buena definición de la tarea y de separación de la lógica. A veces necesitaremos de varios PRs para terminar una tarea. Y casi siempre necesitaremos varios para terminar una historia de usuario.

No es extraño en entornos profesionales rechazar una PR simplemente por el tamaño de la misma.

¿Y cómo de pequeña debe ser? Obviamente, depende de lo que trate la PR. Por ejemplo, si la PR trata de arreglar los espacios de nombres de un Proyecto no está mal que cambie 50 ficheros. Pero como norma no debería tener cambios en más de 12 ficheros.

Hacer solo una cosa por pull request

Al igual que el Principio de Responsabilidad Única de SOLID establece que una clase debe tener una sola responsabilidad, o las tareas de SCRUM deben centrarse en una única acción. Una PR debe tener un solo objetivo.

Imagina, una PR que responde a tres problemas independientes (llamémoslos A, B y C). El revisor puede acordar contigo que A y C son problemas válidos, y que su solución es correcta. Sin embargo, puede no estar de acuerdo con la B. Tal vez piensa que no es un problema o no está de acuerdo con la forma en que se ha solucionado.

Esto se puede convertir en una larga discusión sobre qué ocurre con el problema B, y cómo se está abordando. Esta discusión puede prolongarse durante días (especialmente si os encontráis en diferentes zonas horarias), mientras se intenta llegar a un acuerdo; Se harán cambios en el PR, se generara sinergia y todo esto lleva tiempo. De hecho, puede llevar tanto tiempo que otros PR se han mergeado en nuestra rama de desarrollo y su solicitud de Pull ha caído tanto que ya no se puede fusionar automáticamente. Bienvenidos al Merge Hell.

Todo ese tiempo, las soluciones a los problemas A y C que son perfectamente validas están siendo retenidas. Por esto y más motivos (hacer PR pequeñas, menos riesgo de bloqueo, menos discusión) hacer tres PR independientes para cada una de las soluciones A, B y C arregla todos estos problemas.

Vigilar el ancho de línea

Tanto GitHub, como Stash o VSTS ofrecen visualizaciones de diff para el navegador. Éstos se pueden configurar para visualizar los cambios a un lado y a otro lo que hace mucho más fácil entender los cambios, pero también significa que el código debe ser legible en media pantalla.

Escribiendo líneas demasiado largas, fuerzas al revisor a desplazarse horizontalmente, algo que es un poco incomodo a la hora de revisar.

Afortunadamente las pantallas cada vez mayores y los navegadores con zoom resuelven en parte este problema, pero igualmente hay que cuidar nuestro ancho de línea.

Escribir descripciones y títulos útiles

Escribir una descripción útil en la sección «detalles» de su PR puede ser casi tan eficaz como hacer una PR pequeña. Si no tenemos más remedio que hacer una PR larga, lo mejor es dejar una buena descripción. Tratemos que las descripciones guíen al revisor a través del código tanto como sea posible, resaltando los archivos relacionados y agrupándolos en los conceptos o problemas que se están resolviendo. Esto ahorrara al revisor mucho tiempo ya que no tienen que entender cada fichero para poder agruparlos por ellos mismos e identificar conceptos relacionados. Así es mucho más fácil razonar y revisar nuestro enfoque del problema. No olvidemos poner un título lo más descriptivo posible a la solución planteada en el PR.

Documente su razonamiento

Los comentarios de código son como disculpas, al no poder haber conseguido escribir un código suficientemente auto-explicativo. Todos preferimos métodos, tipos y variables con buenos nombres y estructuras sencillas en vez de comentarios. Sin embargo, al escribir código, muchas veces hay que tomar decisiones que no son evidentes (sobre todo cuando se trata de «lógica de negocios»).

Por eso hay que documentar por qué escribimos el código de la manera en que lo hicimos; No lo que hace el código que escribimos.

Hay distintas formas de explicar nuestro código:

Código autodescriptivo: Lo ideal es que todo nuestro código sea lo suficientemente sencillo y autodescriptivo como para no tener que explicar nada.
La realidad es que esto lo lograremos pocas veces Clean Code es el libro de referencia para escribir código legible. Altamente recomendable leerlo y tenerlo en nuestra oficina o biblioteca personal.

Comentarios en el código: Si no hemos logrado hacer que el código sea suficientemente autodescriptivo, podemos agregar un comentario en el código. El comentario se ubica junto con el código, y debemos poner mucho hincapié en que aporte una explicación útil, ya que si no lo hace estaremos agregando líneas de más para nada. Un comentario inútil lo definiría como un estorbo, de hecho Clean Code desaconseja los comentarios y solo habría que ponerlos en casos necesarios.

Comentarios en la descripción de la PR: En los sistemas de gestión de PRs, como GitHub, VSTS o Stash, podemos agregar descripciones de la propia PR. Es la manera más sencilla de explicar cómo llegamos a nuestra solución. Simplemente abres la PR te lees la descripción y pasas a revisar el código. Lo único malo de esto es que si cambiamos de sistema (Imaginad que pasamos de VSTS a GitHub o BitBucket). Todos los mensajes de explicación del código se perderán.

Agregar comentarios sobre su PR para guiar al revisor: Si solo hemos re-indentado las líneas en un fichero ¿Es este fichero importante en el contexto de la PR? ¿Hay algún archivo relacionado o acoplado con otro en la misma PR? Podemos dejar un comentario en las líneas superiores de estos ficheros que ayuden al revisor a navegar por su PR. Exacto, me refiero a dejar un comentario en el Diff como si fuéramos un revisor más. Hay que limitar estos mensajes porque a veces ensucian la revisión. Pero a veces son muy útiles.

Los comentarios de la PR no deben utilizarse para explicar el código. Si estás explicando tu código en un comentario, podría oler a que el código no está bien estructurado, no es lo suficiente auto explicativo el naming no es correcto o incluso considerar poner un comentario real en el código. No es necesario explicar lo obvio, pero sí considerar un posible error en el lado del revisor más que nada por precaución. Lo que es obvio para ti, puede no ser obvio para nadie más, o incluso no ser obvio para ti mismo en tres meses.

Evitar re-formatear

Es posible que mientras realizas una tarea sientas la necesidad de cambiar el formato del código para ajustar el estilo. No lo hagas por favor.

Cada carácter que cambias en el código fuente aparece en el diff. Algunos visores tienen opciones para ignorar los cambios de espacios en blanco, pero incluso con esta opción activada, hay cosas que los visores no pueden ignorar. En particular, no pueden ignorar si mueves el código, así que por favor no lo hagas.

Por supuesto las tareas de refactoring de estilo o reordenación de código son necesarias. Hacerlas en una PR distinta acelera la revisión ya que te permiten poner el foco en la tarea específica que desarrolla el PR y crea menos confusión. Si quieres solucionar problemas de espacios, indentación, renaming, mover código dentro de los ficheros, cambiar el formato o realizar otros cambios estéticos, hazlo en PR aislada e indica en la descripción que cambios de formato has realizado.

Asegúrese de que todos los tests pasen

Suponiendo nuestra base de código en cuestión tenga pruebas automatizadas, debemos asegurarnos que todas las pruebas pasen antes de crear la PR.

Esto debe convertirse en un hábito. Es decir, existen métodos para que se pase una build al generar una PR de forma automática, pero queda muy feo que esta build nos ponga una etiqueta en la PR diciéndonos que el código falla. La máquina de build estará más sobrecargada y si llega a ocurrir el fallo, no es agradable corregir una PR que ya sabemos de antemano que no es correcta.

Añadir tests

Una vez más, suponiendo que el código en cuestión ya tiene pruebas automatizadas, deberíamos agregar tests para completar nuestra cobertura de código y asegurarnos lo que acabamos de escribir.

Si nuestra DoD (Definition of Done) requiere tests unitarios, es un caso de rechazo de la PR que no los lleve. Es una regla dura, aunque hay varios casos en los que es posible que la PR no lleve tests.

Asegurarse de que el código compila

Antes de crear la PR, compilemos en nuestra máquina. Es verdad que si funciona en nuestra maquina no es lo más útil, pero es lo mínimo. Si no funciona en nuestra máquina, es bastante improbable que funcione en otras máquinas obviamente.

Hay que tener cuidado con los warnings del compilador. No deberían impedir que el código compile, por lo que probablemente no las notemos si no las buscamos explícitamente. Sin embargo, si con nuestra PR estamos generando más warnings, la PR podría ser rechazada.

Si el proyecto tiene un script de compilación, deberíamos intentar ejecutarlo y crear la PR solo si la compilación es correcta. Muchos scripts de compilación tratan las advertencias como errores. Los scripts de compilación también pueden automatizar o implementar varias reglas para esta base de código en particular. El proyecto se compilará igualmente en la fase de mergeo del código del PR en la fase de integración continua por lo que aconsejamos, automatizar el servidor de IC para que asigne una tarea o bug para arreglar la build con prioridad critica si una PR con código que no compila se llegara a intentar mergear.

Escribir bien

Intenta evitar que tus compañeros tengan que poner comentarios de code style, proponer renamings y recordarte que tienes que quitar espacios de más, es una pérdida de tiempo para todos. Hay guías de estilo y linters para eso.

Escribe buen código, pero también escribe de forma correcta. Esto es parcialmente subjetivo. Los linters que proporcionan reglas tanto para el código como para la forma. El código tiene reglas de corrección: si las rompes, no compila (o, para lenguajes interpretados, que fallan en tiempo de ejecución).

Haz que sea visual

Agrega algunas capturas de pantalla de los cambios de front-end. Las capturas de pantalla hacen que el trabajo del revisor sea mucho más fácil. Es más fácil razonar acerca de los cambios visuales cuando se puede ver lo que se ha cambiado en la PR. Si te sientes extra generoso, agrega un GIF o video del uso de la característica (sería genial si lo agregas para varios navegadores, si es una característica clave).

Además, también podemos agregar a los diseñadores para que puedan ofrecer feedback de los cambios front-end. A menudo pueden detectar defectos visuales en el proceso gracias a las capturas de pantalla.

No hagas bajar, actualizar, compilar y ejecutar el proyecto a tu revisor para esto.

Haz Squash

Es una práctica habitual que cada PR contenga 3 o 4 commits, para asegurarnos que podemos deshacer y volver atrás si no nos gusta la solución que estábamos desarrollando o porque hemos parado el desarrollo para atender otras necesidades.

Si cumplimos la regla de las PR pequeñas, normalmente necesitaremos solo un commit por PR. Así que para ello necesitaremos hacer squash de los commits para que todo quede en un único commit y ocultar todo el código fallido que pudiéramos haber generado hasta llegar a nuestra solución.

Conozco gente que a la que le gusta dejar los commits para que los revisores vean como avanzo hasta encontrar la solución al problema. Yo eso lo veo innecesario, ya que la idea es que el revisor nos dé un feedback, de una solución expuesta y para ello no necesita conocer como llego a ella, además de que lo anterior es una pérdida de tiempo para ambos.

En TDD una práctica habitual es hacer un commit para cada uno de los pasos de resolución de la tarea o método. (Red, Green y Refactor). Tampoco necesitamos esto en nuestro PR, ya que no necesitamos saber cómo de mal estaba el código antes de llegar a nuestro refactor.

Si aun así queremos generar PRs con más de un commit, algunas herramientas como VSTS nos permiten hacer el squashing automáticamente. Pero de ningún modo estos commits de generación deberían llegar a nuestras ramas maestras, solo un commit con la tarea terminada.

Aprobados por varios usuarios

Que una PR sea revisada y aprobada por varios compañeros garantiza al menos 3 puntos de vista, mayor porcentaje de encontrar errores y diversas opiniones en caso de que la PR se lleve a debate.

Es obvio que es una gran ventaja. En nuestro equipo no completamos PRs con menos de dos aprobados. Aunque lo normal es que 3, 4 personas lo revisen, aunque solo sea echarle un ojo.

La importancia de los comentarios y discusiones

Los comentarios en el código en nuestras herramientas de Diff (como en un foro) son probablemente lo más importante de la revisión, no solo nos hacen discutir e indicar errores o sugerencias, si no también dejar documentado un cambio. Quiero decir que, si algo se hace de cierta manera o se cambia algo, en los comentarios de la PR quedaría reflejado por qué se llegó a la conclusión de que esto debía ser así.

Así pues, aunque tengamos a los compañeros sentados cerca y podamos preguntarles, siempre es recomendable usar los comentarios y discutir en la propia PR.

Además, acostumbrarse a esto nos hará menos localizados, pudiendo trabajar en remoto con otros equipos o incluso con gente que no conocemos como en GitHub.

Por supuesto esto puede o debe reforzarse con conversación oral siempre que tengamos la oportunidad de trabajar juntos en la misma oficina.

Resolución de Discusiones

A veces cuando un compañero nos insinúa un cambio en nuestro PR y nosotros no lo vemos bien, se produce una discusión que puede durar días, si la feature en cuestión es muy importante. Al final la PR la haces tú, pero el código que quieres agregar lo vas a hacer a un proyecto que es del equipo, de todos. ¿Quién tiene más potestad? Realmente ninguno, tú conoces el código de la PR y sabes que enfoque le has querido dar, pero luego tiene que pasar un proceso de revisión que no te podrás saltar, porque si no, no serviría y que, para eso está.

Es recomendable tener un protocolo de resolución de discusiones.

Lo primero es explicar bien el código y nuestros argumentos de porque debería ser mergeado.

Para empates técnicos en los que los dos, revisor y creador tienen enfoques diferentes pero válidos. Podemos seguir varias estrategias. De mejor a peor estrategia según mi criterio:

  1. Un tercer revisor o un cuarto que ofrezca su postura, escuchando los argumentos de ambos y que rompa el empate.

  2. Jerarquías entre los desarrolladores, los cuales deciden que se debe incluir y que no en un proyecto.

  3. Si son líneas de código triviales, optar por cambiar el desarrollo siempre a la propuesta del revisor o al contrario. No incluir la propuesta en caso de empate en la discusión.

Ser educados y evitar imponer nuestra opinión injustificadamente

A veces, un revisor nos señalará varios problemas en nuestro PR, y estaremos de acuerdo (y agradecidos) en abordarlos. Otra vez no estaremos de acuerdo con los cambios y abriremos una discusión.

Imagina que vas mal de tiempo con tus tareas y te encuentras con que tu PR se llena de comentarios. Podrías pensar que tus compañeros te están sobrecargando de trabajo, a veces, sobre todo cuando no se tiene mucha experiencia con las PR se tiende a ver esto como un mal y aparece resistencia al cambio de la PR (muchas veces a la vista del revisor, de manera injustificada). No es así, el trabajo que realizaste no estaba realmente terminado, solo es así cuando pasa la DoD y esto ocurre también cuando pasas el proceso de revisión. Una de las cosas que se aprenden con los PR es a ser más cuidadoso con el código y esmerarnos más para que luego nuestros revisores no nos hagan enmendar nuestro código.

En el lado contrario, cuando estamos revisando un código, cada uno de nosotros tenemos manías, preferencias de estilo y arquitectura. Intentad evitar imponer nuestras manías y preferencias. Hay que ponerse en contexto del autor de la PR para poder entender que hizo. Y una vez detectada una posible mejora explicar por qué si la incluimos el código del proyecto se vera beneficiado.

Conclusiones

Hemos expuesto aquí algunos consejos a la hora de generar PRs y mejorar la experiencia de los revisores de nuestro equipo. Esta es la clave, mejores PRs implican menor tiempo de revisión y revisiones más afinadas. Así que, si tenemos 2, 3 o 4 revisores implicados, la ganancia en tiempo del equipo se optimice por dos, por tres o por cuatro y evitar que nuestros compañeros vean la fase de revisión como una gran inversión de horas.

Hay que enfatizar que esto son solo consejos, no reglas duras e inflexibles.
Solo no hagas que su revisor esté incomodo o pierda tiempo con la revisión de tu código.
Al final como casi todo es cuestión de experiencia y práctica. Prueba y error.

No dudéis en comentar si tenéis algún que otro consejo para escribir buenos PRs.

Fuentes:
– 10 tips for better Pull Requests by Mark Seemann
– The (written) unwritten guide to pull requests by Blake Riosa
– Y un poquito de nuestra experiencia

Kenny R.