Cambios que no tenían nada que ver con el ticket
«Ya que estaba, aproveché y arreglé esto.» Es de buena fe y hace que la review se vuelva imposible, que el revert sea riesgoso y que la causa de un bug quede escondida entre veinte archivos que nadie pidió.
Un cambio que debía tocar dos archivos toca veintitrés. Diecinueve son renombres, un formateo y una mejora que a uno le pareció obvia mientras leía. Todo correcto, todo bien intencionado.
Y sin embargo es de los comentarios de review más firmes que uno recibe: «¿esto entra en el ticket?». No es burocracia. Un pull request mezclado tiene tres costos concretos.
Los tres costos
| Costo | Cómo se manifiesta |
|---|---|
| La review pierde profundidad | El revisor no distingue lo que arregla el bug de lo que es cosmético, y termina aprobando por volumen |
| El revert deja de ser una opción | Si algo falla en producción, volver atrás también revierte cinco arreglos que estaban bien |
| El historial miente | Buscar cuándo se introdujo un comportamiento lleva a un commit que dice otra cosa |
El del medio es el que más duele en un incidente. La decisión de revertir se toma con presión y en minutos: si revertir implica evaluar qué más se va a deshacer, se termina eligiendo arreglar hacia adelante con menos información.
Qué sí puede entrar
La regla no es “nunca toques nada más”. Hay cosas que pertenecen naturalmente al cambio.
Dentro del alcance
- Lo que el cambio necesita para existir. Extraer una función para poder usarla, agregar el tipo que faltaba, adaptar al llamador.
- Lo que quedó muerto por el cambio. Si tu modificación dejó código sin usar, borrarlo es parte del trabajo, no una limpieza aparte.
- El test del comportamiento que tocaste. Siempre.
- Un arreglo trivial y evidente en la línea que ya estabas editando, si es de una línea y no cambia comportamiento.
Cómo separar sin perder el trabajo
La objeción práctica es real: uno ya lo escribió, separarlo cuesta. Hay formas baratas.
Tres maneras
- Dos pull requests, el de limpieza primero. El de refactor sin cambio de comportamiento se revisa rápido porque se lee distinto; el de lógica queda chico y legible encima.
- Commits separados dentro del mismo pull request. Peor que lo anterior y mucho mejor que nada: permite revisar por commit y revertir uno solo.
- Anotarlo y dejarlo. La mejora que se ve al pasar no tiene por qué hacerse ahora. Un ticket de treinta segundos conserva la observación sin ensuciar el cambio.
Antes de seguir, predecí
El tamaño como señal
Hay una correlación que se verifica sola: cuanto más grande el pull request, más superficiales los comentarios. Arriba de cierto umbral, la review deja de encontrar problemas de lógica y empieza a comentar cosas de forma, porque es lo único que se puede sostener leyendo.
Cuando un cambio es genuinamente grande —una funcionalidad nueva completa—, lo que ayuda es el orden de lectura: dejar en la descripción por dónde empezar y qué archivos son mecánicos. Es la diferencia entre una review de verdad y un sello.
Más a fondo · nivel seniorEl refactor que hay que hacer para que el cambio sea posible
Hay un caso legítimo donde el refactor no se puede separar: cuando el cambio pedido es imposible sin reordenar antes. Ahí la forma correcta es el refactor primero, en su propio pull request, sin cambiar comportamiento y con los tests existentes en verde como prueba. Después, el cambio de comportamiento encima, chiquito. Cuesta una ronda más y es lo que permite revertir con precisión.
Un pull request mezclado
Hay 4 problemas en este cambio. Tocá la línea donde creas que está.
| 1 | |||
| 2 | 1 | ||
| 2 | |||
| 3 | |||
| 3 | 4 | ||
| 4 | |||
| 5 | |||
| 5 | |||
BloqueaEl renombre arrastra a todos los que llaman a esta función Cambiar buildReport por buildMonthlyReport puede estar muy bien y toca todos los archivos que la usan. Mezclado con un arreglo de seguridad, obliga a quien revisa a leer veinte archivos para encontrar la línea que importa, y el riesgo es que la apruebe por cansancio. | |||
| 6 | |||
BloqueaUna mejora que nadie pidió, y que además cambia el comportamiento Agregar caché en el mismo cambio que arregla un filtro por cuenta es peligroso de verdad: si la clave del caché no incluye la cuenta —y acá no la incluye—, el caché vuelve a filtrar datos entre clientes, justo lo que se estaba arreglando. Dos cambios chicos y correctos por separado que juntos reintroducen el bug. | |||
| 7 | |||
| 8 | |||
| 9 | |||
| 10 | |||
BloqueaEsto es el ticket, y es lo único que lo es Agregar el filtro por cuenta es el arreglo: sin él, un reporte mostraba facturas de otros clientes. Es un cambio de una línea, de seguridad, y es lo que hay que revisar con atención. En este pull request está al final, después de tres cambios que no tienen nada que ver. | |||
| 6 | 11 | ||
| 7 | 12 | ||
| 8 | 13 | ||
Cuatro cambios, uno solo es el ticket, y el tercero reintroduce el bug que el primero arregla.
Cómo se mantiene chico
Cierre
Autoevaluación
¿Lo entendiste?
Práctica
SugerenciaEl reordenamiento de imports ensucia el diff
Probablemente lo hizo el editor al guardar. No rompe nada y agrega ruido al diff, que es el costo que se paga en atención de quien revisa. Va aparte, o mejor, va en una regla del formateador para que no lo decida cada uno.