Pull Requests: Cómo crear PRs de calidad
Una Pull Request es la unidad mínima de trabajo en un equipo de desarrollo de software. Si un desarrollador se pasa semanas “trabajando” y no ha enviado ninguna PR al equipo para revisar… algo huele mal: hasta que la PR no se revisa y se mergea, ese trabajo no sirve al equipo para nada, es como si no se hubiera hecho nada… durante semanas!
Las PRs marcan el ritmo de desarrollo de un equipo de desarrollo. Un equipo que funciona bien tiene un ritmo constante de muchas PRs pequeñas que se revisan y mergean con frecuencia (varias veces al día).
También te puede interesar “Commits, lo estás haciendo mal [posiblemente]”
Características de una buena Pull Request
Una buena PR contiene una sola cosa
Una PR debe contener solo commits relacionados con un solo motivo de cambio:
- un solo motivo de cambio puede afectar a varios ficheros y necesitar varios commits: p.ej. si cambiamos el nombre de una función, seguramente tendremos que tocar todos los puntos del código en los que se usa esa función, y los tests asociados.
- una PR con un solo “cambio” es más fácil de revisar, ya que no hay que tener en la cabeza el contexto de dos o tres cambios distintos
- si una PR contiene dos o tres cambios (A, B y C) se podría dar el caso de que los revisores estén totalmente de acuerdo con A y C pero tengan reparos sobre B. A lo mejor la PR da lugar a un debate sobre si B debe hacerse o la mejor forma de implementar B… y mientras ese debate se resuelve, A y C están “atrapados” en la PR sin poderse poner en producción
- si tenemos que “revertir” un cambio, es mucho más fácil hacerlo si tenemos cada cambio en una PR distinta
Es fácil decir “una PR debe contener un solo cambio”, pero la realidad es que cuando estamos desarrollando casi siempre estamos haciendo varias cosas a la vez. P.ej. mientras añadimos una nueva feature, descubrimos un bug en una de las funciones que estamos tocando, o decidimos que sería mejor refactorizar un par de funciones para que la nueva feature sea más sencilla de añadir… en esos casos, tenemos que pararnos y aplicar “divide y vencerás”. Como dice Martin Fowler, podemos cambiarnos el sombrero (feature/bugfix/refactor) todas las veces que queramos, pero solo podemos llevar un sombrero a la vez. Lo correcto en estos casos es abrir otras ramas a partir de development y hacer los cambios en PRs independientes que se revisarán y mergearán por separado. ¿Qué pasa si los cambios son interdependientes? (necesito unos cambios para poder hacer los siguientes) Hay una forma de solucionarlo que explicamos más abajo en el apartado “¿Qué hago mientras mi PR está siendo revisada?”
Una buena PR es “pequeña”
Hemos dicho que un equipo que funciona bien produce un flujo constante de PRs pequeñas que se mergean frecuentemente. Solo podemos hacer muchas PRs y revisarlas rápidamente si las PRs son pequeñas. Que además será una consecuencia natural de haber tenido en cuenta el punto anterior: es raro que una PR sea demasiado grande si hace una sola cosa y hemos aplicado el “divide y vencerás”.
A veces, nos venimos arriba y nos sale una PR “muy grande”: lo mejor en este caso es aparcarla (p.ej. la ponemos en modo Draft, sin revisores) y la vamos troceando en PRs más pequeñitas con los distintos cambios individuales (divide y vencerás a posteriori).
Una buena PR funciona
Una PR contiene código que se va a desplegar a producción y que van a utilizar el resto de compañeros del equipo para su trabajo individual. La primera regla del trabajo en equipo es “no romper nada”.
Por muy trivial que parezca, tenemos que garantizar que el cambio que estamos subiendo no rompe nada. No sería la primera vez que un cambio aparentemente inocente en una línea de código hace que producción “estalle” porque falta un import, o que la app deje de funcionar para el resto de miembros del equipo.
Para ello, tenemos el deber de ejecutar todo el código que estamos subiendo, ya sea a través de pruebas unitarias (idealmente automatizadas en el CI/CD) o mediante ejecución manual en nuestro entorno de desarrollo. Una linea de código nunca se debería ejecutar por primera vez en producción.
Una buena PR contiene una explicación de “Por qué”
Cuando estamos desarrollando, tenemos en la cabeza el contexto de por qué estamos haciendo ese cambio: hay un bug que alguien ha reportado en un issue de JIRA, hemos identificado un refactor que nos viene bien, estamos desarrollando una feature…
Ese contexto es esencial para entender los cambios que se proponen al código, y es necesario tanto para revisar correctamente una PR como para depurar problemas en el futuro. ¿Qué hacemos si una determinada línea falla dentro de 6 meses?… o visto de otra forma… si hoy estamos buscando un bug e identificamos un fallo en una línea que se cambió hace 8 meses ¿no nos ayudaría tener una lista de los cambios que se hicieron a la vez y una explicación de por qué?
Es importante hacer referencia a JIRA, a otras PRs relacionadas, copiar una discusión de slack que hemos tenido al respecto, un documento, un artículo de internet que hemos seguido… si capturamos el contexto de desarrollo actual, será mucho más fácil entender el cambio en la revisión y solucionar los problemas (que seguro van a salir) en el futuro.
Buenas Prácticas
Revisa tu propia PR
Es una costumbre que require poco esfuerzo (5–10 minutos) y que ayuda a mejorar muchísimo la calidad de las PRs que proponemos para revisión.
Antes de poner a otros como revisores, podemos revisar nuestra propia PR, poniéndonos en el lugar de los revisores. De esta forma podemos detectar cosas que no están claras, cambios que se han subido sin querer, cambios no relacionados… es normal añadir uno o dos commits más durante esta auto-revisión, para mejorar la legibilidad del código, añadir o eliminar comentarios… Incluso podemos dejar comentarios en la PR para guiar la revisión, o pedir opinión a los revisores sobre algún punto en concreto.
Una PR es un debate, y como autores de la PR podemos orientar ese debate y hacer las revisiones más rápidas efectivas.
PRs Draft y Etiquetas
Una PR es un buen mecanismo de comunicación o debate sobre cualquier código. Además de revisar código listo para mergear, también es muy útil para revisar o debatir ideas, o incluso pedir ayuda a otros compañeros. Tan pronto como empecemos una rama para trabajar en algo, podemos abrir una PR “Draft” para ir enseñando nuestro código a los demás. Podemos añadir commits (y hacer git push) tantas veces como nos haga falta mientras seguimos trabajando, y podemos utilizar la interfaz de GitHub para pedir opinión, ayuda o recibir feedback y sugerencias.
Otras veces, queremos pedir al equipo que revise el código pero queremos esperar para mergearlo (p.ej. porque tenemos que hacer algo manualmente antes de desplegar). En esos casos, podemos poner una etiqueta “DON’T MERGE YET” para que el último revisor no haga el merge.
El último revisor en aprobar hace el merge
El código aprobado está listo para mergear, por tanto, el último revisor que aprueba una PR puede y debe hacer el merge inmediatamente.
¿Qué hago mientras mi PR está siendo revisada? ¿puedo seguir trabajando en cambios sucesivos?
Una de las objeciones más frecuentes a la estrategia de “divide y vencerás” es que la mayoría de las veces, los cambios que queremos hacer deben ir en orden. Por ejemplo, si quiero implementar una feature y para ello primero voy a hacer un refactor… lógicamente para hacer los cambios de la feature necesito tener los cambios del refactor incorporados en el código…
La solución es “encadenar” PRs:
- partiendo de la rama
development, creamos la primera ramarefactor/one-refactory hacemos los cambios que necesitemos. Hacemosgit pushy abrimos una PR para revisar y aprobar esos cambios. - Mientras los revisores se toman su tiempo para revisar el código, no hace falta esperar, podemos seguir trabajando en la feature.
- Simplemente creamos otra rama
feature/one-featurepartiendo de la ramarefactor/one-refactor. Así tendremos todos los cambios anteriores del refactor ya disponibles para implementar la feature.
Cuando termine la feature, puedo hacer git push y abrir una nueva PR, teniendo cuidado de seleccionar como target la rama refactor/one-refactor(y mantener el principio de que una rama se debería mergear a la misma rama de la cual nació)
Ahora, los revisores tienen dos PRs para revisar: la del refactor y la de la feature. Pueden revisar y aprobar las PRs en cualquier orden: en una verán los cambios del refactor y en la otra los cambios de la feature.
Si aprueban primero el refactor, esos cambios se mergearán a development y la PR de la feature cambiará automáticamente su target a development
Si aprueban primero la feature, los cambios se mergearán a la rama del refactor, y cuando esta PR se apruebe entrarán todos juntos a development.
En ambos casos, el resultado es el esperado: los cambios terminan en development en el orden correcto.
Referencias
Revisión de PRs
- Respeto y empatía
- Agilidad
Ideas erróneas sobre las PRs
- No tengo que abrir la PR hasta que mi código esté terminado/perfecto
- Una PR es la foto “final” de un cambio
- Me da vergüenza abrir una PR
- Me da vergüenza poner un comentario en una PR
