Guías de Revisión de Código
Qué buscan los revisores en las pull requests de Python: corrección, pruebas, seguridad, operabilidad y convenciones del equipo, sin convertir la revisión en minucias de estilo que ruff ya aplica.
Receta
Pase de 5 minutos del revisor:
- Leer la descripción del PR: enlace a la tarea, nota de migración, plan de implementación.
- Escanear la lista de archivos: dependencias inesperadas, archivo de bloqueo, alembic, infraestructura.
- Leer primero las pruebas: ¿se cubre el cambio de comportamiento?
- Comprobar la implementación: casos límite, errores, tipos.
- Comentar con preguntas, no con exigencias: aprobar cuando haya confianza y CI en verde.
Cuándo consultar esto:
- Autor preparando el PR para revisión
- Revisor abriendo la cola de notificaciones
- Líder técnico calibrando la cultura de revisión
- Retrospectiva después de un defecto escapado
Ejemplo Funcional
Buena descripción de PR:
## [BILL-91] Aplicar IVA a pedidos de la UE
- Añade TaxService con aritmética Decimal
- Revisión de Alembic `a1b2c3` añade la columna `vat_amount` (nullable, rellenada en un seguimiento)
- Bandera de funcionalidad `billing.vat_enabled` desactivada por defecto
## Plan de pruebas
- [x] uv run pytest tests/billing/test_tax.py
- [x] Manual: crear pedido con dirección DE, activar bandera, ver línea de IVA
## Reversión
Desactivar bandera; migración reversible mediante downgrade (elimina la columna - OK pre-producción)Buen comentario de revisión:
**Pregunta:** ¿Qué sucede con los elementos de línea de cantidad cero? ¿Se divide por cero o se omite?
Sugiero afirmar en `test_tax_zero_quantity` - estaré encantado de aprobar después de eso.Lo que esto demuestra:
- La descripción enlaza la tarea, la migración, la bandera y la reversión
- El plan de pruebas refleja los comandos de CI
- La revisión hace una pregunta + sugerencia de prueba concreta
- Se mencionan explícitamente Decimal y la bandera de funcionalidad
Inmersión Profunda
Cómo Funciona
- Responsabilidad del autor - PRs pequeños, autoevaluación del diff, CI en verde antes de solicitar revisión.
- Responsabilidad del revisor - Respuesta oportuna, priorizar el riesgo, aprobar cuando se cumpla el listón.
- CODEOWNERS - Asignación automática de expertos en dominios para rutas sensibles.
- CI como primer revisor - ruff, pytest, mypy, escaneo de seguridad.
Lente de Revisión (orden de prioridad)
| Prioridad | Comprobación |
|---|---|
| P0 | Corrección, seguridad, riesgo de pérdida de datos |
| P1 | Pruebas, migraciones, rupturas de contrato de API |
| P2 | Rendimiento, observabilidad (logs/métricas) |
| P3 | Estilo (si CI lo omitió), nombres, documentación |
Enfoque Específico de Python
- Async - ¿Llamadas bloqueantes en
async def? - Dinero - ¿Se usa
floatpara la moneda? - SQL - ¿Solo consultas parametrizadas?
- Carga de Pickle/yaml - ¿Deserialización insegura?
- Dependencias - ¿Nuevos paquetes transitivos justificados en el diff del archivo de bloqueo?
- Tipado - ¿API pública anotada?
Trampas
- Aprobación ciega de CI en verde - Las pruebas pueden no cubrir la rama. Solución: leer las aserciones de las pruebas, no solo la cobertura %.
- Comentarios de estilo que ruff aplica - Pérdida de tiempo del autor. Solución: comentar solo si la regla debe cambiar en pyproject.
- Bloqueo por preferencia - "Usaría
dictnodataclass". Solución: pedir ADR si es arquitectónico. - PRs enormes - 50 archivos imposibles de revisar. Solución: el autor divide; el revisor solicita dividir antes de una lectura profunda.
- Revisión de migración faltante - Despliegue falla en revisión duplicada. Solución: comprobar la cadena de alembic y la nota de reversión.
- Tono poco amable - Aumenta el factor autobús. Solución: redacción colaborativa; elogiar patrones buenos.
Alternativas
| Alternativa | Usar Cuando | No Usar Cuando |
|---|---|---|
| Programación en parejas | Cambio complejo y arriesgado | Diferencia horaria para revisión asíncrona |
| Bot de revisión IA | Primer pase de minucias | Revisor único para seguridad |
| Documento de diseño antes del código | Gran cambio arquitectónico | Corrección de un error de una línea |
| Sin revisión (individual) | Prototipo desechable | main de producción |
Preguntas Frecuentes
¿Qué tan rápido debe ser la revisión?
Objetivo de primera respuesta < 1 día hábil; SLA según la wiki del equipo.
¿Aprobar con minucias?
Sí, si las minucias no son bloqueantes y se rastrean opcionalmente en una tarea de seguimiento.
¿Solicitar cambios vs comentar?
Solicitar cambios cuando la fusión causaría riesgo en producción; comentar para fines educativos u opcionales.
¿Junior revisa a senior?
Se anima - las preguntas mejoran la documentación; el senior sigue siendo CODEOWNER en rutas críticas.
¿Autofusión en verde?
OK para actualizaciones de parches de dependabot con política; no para migraciones sin intervención humana.
¿Revisar notebooks?
Preferir .py emparejados de Jupytext o revisar solo el diff con salidas eliminadas.
¿Desencadenante de revisión de seguridad?
Autenticación, criptografía, nuevos analizadores de entrada externa, aumentos importantes de dependencias - etiquetar al compañero de seguridad.
¿Desencadenante de revisión de rendimiento?
Consultas de ruta crítica, nuevos bucles ORM N+1, pandas grandes en la ruta de solicitud.
¿Quién fusiona?
El autor después de la aprobación + CI en verde, a menos que el rol de gestor de lanzamiento para el tren.
¿Monitorización post-fusión?
El autor vigila los paneles 30 minutos después del despliegue de su cambio - anotarlo en la plantilla del PR.
Relacionado
- Convenciones y Guía de Estilo - modismos
- Mejores Prácticas de Git - higiene de PR
- Cultura de Revisión de Código - liderazgo
- 30 Reglas de Seguridad para Python - lente de seguridad
Versiones de Stack: Esta página fue escrita para Python 3.14.0 (estable 3.14, mantenimiento 3.13), FastAPI 0.115+, Django 5.2, Flask 3.1, Pydantic 2, PyTorch 2.6+, pandas 2.2+, Polars 1.x, ruff 0.9+, y uv 0.6+.