El review con orden (el PR de la feature de recordatorios): el QUÉ (descripción con alcance), contrato (sin cambios de API), seguridad (el recordatorio no filtra), corrección (el bucle consulta la tarifa por fila: el N+1 del 09 — BLOQUEANTE), tests ~ (el test no cubre el caso sin tarifa — debería), estilo (2 nits: nombre de variable, orden de imports). La clasificación: 1 bloqueante, 1 debería, 2 nits — el PR de "5 problemas" era de 1+1.
El espejo: revisando por el estilo primero: 7 comentarios de estilo y el N+1 apareció en el minuto 25 (si apareció) — la comparación documentada: el orden es el 80% de la eficacia del review; el estilo-first produce el review de pánico del "LGTM con 7 nits".
El ADN cazó lo que el review normal no vio: el PR usaba cache.get de disponibilidad dentro del checkout (la 38 prohíbe: la 22 del ADN "lock/decisión en TX") — el review "normal" lo pasó (el caché parece inocente); la pregunta del ADN ("¿el caché decide la compra?") lo caza.
Ejercicio 2 — Los comentarios que enseñan
Las reescrituras:
"Esto es un N+1 horrible."
→ "Este loop lanza 1 query por reserva: con 40 son 83 queries (el 09).
prefetch_related lo deja en 3 y el test de listado en verde. ¿Bloquea tu caso?"
"Nadie hace esto así."
→ "El repo centraliza los errores en el handler (26): este try/except de vista
duplica el formato. Mover a DomainError mantiene el problem+json consistente."
"¿Por qué no usaste X?"
→ "¿Consideraste select_for_update aquí? Con dos requests simultáneos el saldo
puede quedar negativo (la 10). Si hay razón para no usarlo, ¿documentamos por qué?"
"Este test no prueba nada."
→ "Este test verifica que se llamó a `save` (el CÓMO): con el refactor del 28 muere
sin que el comportamiento cambie. Assert sobre el estado final (status=CONFIRMED)
probaría el QUÉ y sobreviviría."
"El código legacy era mejor."
→ "La versión anterior evitaba el outbox en el mismo commit (25): si la atomicidad
cambió a propósito, ¿el test de atomicidad del outbox lo cubre? Si no, el rollback
del outbox se pierde — ¿lo revisamos?"
La pregunta que enseña (el hallazgo real): "¿Qué pasa si dos requests confirman el mismo intent a la vez?" → respondida: el segundo pasa la máquina de estados si no hay lock (la 10) → el fix: select_for_update en confirmar_intent + el test de la disputa (34). La pregunta del review fue el review entero.
La honestidad del marcado: de 9 observaciones del último PR: 2 bloqueantes reales, 3 debería, 4 nits — 4 escritas con tono de bloqueante. El marcado honesto (nit: delante) habría ahorrado al autor la batalla por el nombre de la variable y la conversación por lo que importaba.
Ejercicio 3 — El desacuerdo con datos
La respuesta del autor:
markdown
"Mantengo el manager directo para Venue/Seat: el ADR de la 25 lo decide (repositorio
para AGREGADOS con lógica; manager para CRUD trivial). Venue no tiene invariantes ni
locks: el repositorio sería un wrapper del ORM con menos features. Si Venue crece
invariantes, abrimos el repositorio con el test que lo exija. El benchmark del 36
(la meseta del browse) no cambia: la capa extra no mejora números.
Si quieres lo reabrimos con un ADR nuevo (48): el estándar es el documento, no mi gusto."
El plan de resolución en 4 pasos: (1) identificar la DISCUSIÓN real (¿estándar del repo o gusto? — el docs/review.md lo separa); (2) si es estándar: el ADR existente decide (o se crea el ADR nuevo con contexto y números); (3) si es técnico-incierto: el benchmark/prototype de 30 min (el dato decide, no el volumen); (4) la decisión documentada se respeta en el PR y se revisa con su trigger — el review no lo re-decide cada sprint.
El PR pequeño: los 3 PRs (el modelo+admin, el servicio+tests, el endpoint+contrato) reciben reviews REALES (el revisor lee 400 líneas con contexto vs 2000 con pánico) — el cambio: el PR de 2000 líneas recibe "LGTM" (que significa "no leí"); los 3 pequeños reciben el N+1 y el lock que el grande escondía.
Ejercicio 4 — El self-review
El self-review del último PR (la saga de la 32): falló en 2 ítems: el alcance sin inside/outside (el "reanudar sagas" no estaba en la descripción) y el test de atomicidad del outbox no entró en el PR (quedó para "después" — el después eterno). El ítem más roto históricamente: el self-review mismo (no ejecutado): la regla personal: el self-review SE EJECUTA (30 min) antes del merge, no se promete.
El diff con ojos de extraño: el test test_saga_2 no documenta el comportamiento (el nombre del 33 falló) → renombrado a test_saga_reanudada_no_duplica_cargo; la descripción del PR ganó el "quién/qué/cuándo" del incidente hipotético. El extraño (el tú del 47) ahora lee el diff sin preguntar.
El review virtual del equipo de 1: la checklist del §5 + el linter (41) + el ADR previo (48) + la regla nueva que el ejercicio reveló: las 24 h de espera (el PR propio abierto 24 h antes del merge propio: el cerebro del día siguiente caza lo que el del merge no ve — el "revisor del mañana" es el único segundo par de ojos gratuito que existe).
Ejercicio 5 — La cultura del review
El docs/review.md (extracto): escala (bloqueante: contrato/seguridad/dinero · debería: N+1/test crítico · nit: estilo · pregunta: diálogo) → orden del §1 → las dos checklist (autor/revisor) → tiempos (review <4 h, re-review <2 h) → la regla del desacuerdo (ADR o benchmark). Una página.
El caso difícil: la respuesta del autor: "El enfoque que propones es válido pero decide 3 estándares no acordados (¿están en el ADR?); el PR funciona y cubre el caso — propongo: merge con los estándares actuales y los nuevos al ADR/review.md para el PR siguiente". El revisor ideal: "Correcto: mis preferencias no son estándar hasta que el repo las adopte — abro el ADR y el siguiente PR los lleva". La línea: el estándar existe si está documentado (ADR/review.md); si no, es gusto — y el gusto se propone, no se impone.
La regla personal (extracto): "Reviso en orden de impacto y clasifico antes de escribir; el comentario lleva hecho+impacto+camino o pregunta; el desacuerdo se resuelve con datos o ADR, jamás con volumen; el self-review se ejecuta, no se promete."
Resumen del profesor
El review se lee por impacto (QUÉ→contrato→seguridad→corrección→tests→estilo); los ADNs del repo son las 5 preguntas que mantienen el sistema.
El comentario adulto: hecho+impacto+camino, o pregunta; la escala (bloqueante/debería/nit/pregunta) evita la guerra de nits.
El autor responde todo y discrepa con datos; el desacuerdo se cierra con ADR o benchmark; el self-review del equipo de 1 se EJECUTA (y las 24 h de espera son el segundo par de ojos).
The Professor