8. Proyecto: escribe comentarios de revisión con vocabulario

Descripción

En este proyecto vas a hacer lo que promete la guía entera: recibes un pull request real sobre Boletia y escribes la revisión completa. No refactorizas nada, no tocas una línea de código. Entregas texto: una serie de comentarios, cada uno con su olor nombrado, su ubicación exacta, su consecuencia concreta, una dirección posible y su peso; más un comentario general del PR y un veredicto justificado.

La prueba con la que se juzga es una sola, y quiero que la tengas delante todo el tiempo: ¿podría el autor actuar sobre cada comentario sin volver a preguntarte nada? No importa si tu revisión es elegante, si encontraste más problemas que nadie o si citaste bien los nombres. Importa si el lunes por la mañana, con tu revisión abierta y sin ti disponible, esa persona sabe exactamente qué hacer con cada punto.

Esto importa porque es, literalmente, el trabajo. En cualquier equipo de software vas a pasar más horas leyendo código ajeno y opinando sobre él que escribiendo código propio. Y de todo lo que has aprendido en esta guía, la revisión es donde se nota primero: alguien que puede decir "esto es shotgun surgery, toca cinco archivos y uno falla en silencio" en el segundo mes de su primer trabajo se distingue de inmediato, no por saber más patrones, sino por poder hacer útil lo que ve.

Conexión con el módulo: este proyecto usa las siete lecciones a la vez. La 2 para reconocer los olores, la 3 para los tres grandes con su detección contable, la 4 para el formato de cinco partes, la 5 para los anti-patrones, la 6 para que cada dirección tenga un primer paso realista, y la 7 para el registro y el tono de cada comentario. Y es el puente al módulo 8: ahí vas a tomar hallazgos como estos y arreglarlos, en pasos pequeños, defendiendo cada decisión con su tradeoff. Aquí nombras; allá refactorizas.

Revisar una obra que no dirigiste

Alguien te pide que revises una remodelación antes de firmar la recepción. No la dirigiste, no elegiste los materiales, no estuviste cuando se decidió mover la pared.

Hay tres formas de hacerlo mal.

Firmar sin mirar, porque el albañil es buena persona y ya está cansado. Es lo más fácil y traslada el problema a quien viva ahí.

Rehacer la obra en la cabeza: "yo hubiera puesto la cocina del otro lado". Puede que sí. No es el encargo, y además el dueño quizá tenía una razón para pedirla ahí.

Anotar cuarenta observaciones sin distinguir un enchufe suelto de una viga mal apoyada. El albañil recibe una lista imposible, arregla los enchufes que son visibles y rápidos, y la viga sigue mal.

La forma que funciona es la del cuarto revisor. Recorre, anota lo que ve con su ubicación, separa lo que impide recibir la obra de lo que se puede arreglar después, y para cada cosa dice qué implica: "el enchufe de la cocina quedó a diez centímetros del fregadero — norma dice treinta; hay que moverlo antes de que entre agua". El albañil sabe qué hacer con cada punto, y sabe cuáles son hoy.

Fíjate en la palabra que hace todo el trabajo: separar. No es encontrar menos cosas ni ser más amable. Es que la lista llegue ordenada por lo que hay que hacer con ella.

El encargo

Llega un miércoles por la mañana, en el canal del equipo:

Dani — "Ya está listo el PR de Klarpay 🎉 Es el proveedor nuevo que pidió comercial para el Festival Cumbre, con el reporte de conciliación que quería finanzas. Quedó grandecito porque las dos cosas iban juntas. ¿Alguien lo revisa? Necesitamos mergear antes del viernes para probarlo en staging la semana que viene."

Contexto que conviene tener antes de abrirlo, porque cambia cómo se revisa:

• Dani lleva cuatro meses en el equipo. Es su primer cambio grande sobre el checkout. Es buena programadora y todavía no conoce toda la historia del sistema.
• Hay una fecha real. El Festival Cumbre es en tres semanas, y el proveedor nuevo es un requisito comercial, no un capricho.
• El PR mezcla dos cosas: un proveedor de pago y un reporte. Eso ya es información.
• Tú llevas más tiempo en el equipo. Sabes de dónde viene el registro de plugins, sabes que checkout.py es un God object heredado, y sabes que el Singleton de config.py viene de la primera versión del sistema.

Ese último punto es el que más pesa en cómo escribes. Buena parte de lo que vas a encontrar no lo introdujo Dani: lo heredó. Confundir las dos cosas es el error más caro de esta revisión.

El pull request

PR #412 — "Agrega Klarpay como cuarto proveedor de pago + reporte de conciliación" Autora: Dani · 7 archivos · +341 / −18

Archivo 1 — payments/klarpay_provider.py (nuevo)

# Archivo: payments/klarpay_provider.py
from klarpay_sdk import KlarpayClient
from config import settings


class KlarpayProvider:
    """Cobra a través de Klarpay."""

    def do_payment(self, amount, order_id, currency="MXN"):
        # El SDK se construye aquí porque necesita la key de settings.
        client = KlarpayClient(
            key=settings.KLARPAY_KEY,
            mode=settings.KLARPAY_MODE,
            timeout=30,
        )
        response = client.charge_card(
            value=amount,
            ref=str(order_id),
            curr=currency,
        )
        return response

    def do_refund(self, payment_id):
        client = KlarpayClient(
            key=settings.KLARPAY_KEY,
            mode=settings.KLARPAY_MODE,
            timeout=30,
        )
        return client.reverse(payment_id)

Para comparar, así son los otros tres (ya en master):

# Archivo: payments/provider.py
class PaymentProvider:
    def charge(self, order): raise NotImplementedError
    def refund(self, order): raise NotImplementedError


# Archivo: payments/stripe_provider.py
class StripeProvider(PaymentProvider):
    def __init__(self, client):
        self.client = client

    def charge(self, order):
        # El SDK habla en centavos y devuelve su propio diccionario.
        r = self.client.create_charge(amount=int(order.total * 100), currency="MXN")
        return ChargeResult(ok=r["status"] == "succeeded", reference=r["id"])

Archivo 2 — checkout/checkout.py (+62 / −2)

# Archivo: checkout/checkout.py   (fragmento con el diff)

    # ---- 3. Cobro ---------------------------------------------------
    if order.provider == "stripe":
        provider = StripeProvider(StripeClient(api_key=settings.STRIPE_KEY))
    elif order.provider == "mercadopago":
        provider = MercadoPagoProvider(MercadoPagoClient(token=settings.MP_TOKEN))
    elif order.provider == "cash":
        provider = CashProvider()
+   elif order.provider == "klarpay":
+       # Klarpay cobra comisión escalonada según el monto; hay que
+       # sumarla al total ANTES de cobrar.
+       if order.total < 500:
+           commission = order.total * 0.045
+       elif order.total < 2000:
+           commission = order.total * 0.032
+       else:
+           commission = order.total * 0.025 + 3.50
+       order.total = round(order.total + commission, 2)
+       repository.save_order(order)
+
+       kp = KlarpayProvider()
+       raw = kp.do_payment(order.total, order.id)
+       result = ChargeResult(ok=raw["state"] == "OK", reference=raw["tx"])
    else:
        raise ValueError(f"Proveedor desconocido: {order.provider}")

-   result = provider.charge(order)
+   if order.provider != "klarpay":
+       result = provider.charge(order)
+
    if not result.ok:
        order.status = "cancelled"
        repository.save_order(order)
        raise PaymentFailed(order.id, result.reference)

    order.status = "paid"
    repository.save_order(order)

+   # ---- 3.5 Conciliación -------------------------------------------
+   # Finanzas necesita una fila por cobro para el corte diario.
+   if order.provider == "klarpay":
+       db.execute(
+           "INSERT INTO reconciliation (order_id, provider, amount, tx, created_at) "
+           "VALUES (?, ?, ?, ?, ?)",
+           order.id, order.provider, order.total, result.reference, now(),
+       )
+
    # ---- 4. Avisos ---------------------------------------------------

Archivo 3 — config.py (+9)

# Archivo: config.py

class Settings:
    _instance = None

    def __new__(cls):
        if cls._instance is None:
            cls._instance = super().__new__(cls)
            cls._instance._load_from_env()
        return cls._instance

    def _load_from_env(self):
        self.STRIPE_KEY = os.environ["STRIPE_KEY"]
        self.MP_TOKEN = os.environ["MP_TOKEN"]
        self.SEATING_PLUGIN = os.environ.get("SEATING_PLUGIN", "default_seating")
+       self.KLARPAY_KEY = os.environ["KLARPAY_KEY"]
+       self.KLARPAY_MODE = os.environ.get("KLARPAY_MODE", "sandbox")
+
+   def is_klarpay_enabled(self):
+       # Se puede apagar Klarpay desde la tabla de feature flags sin desplegar.
+       row = db.query_one("SELECT enabled FROM feature_flags WHERE name = 'klarpay'")
+       return bool(row and row["enabled"])

settings = Settings()      # importado en once archivos distintos

Archivo 4 — api/routes.py (+3 / −1)

# Archivo: api/routes.py

- ALLOWED_PROVIDERS = ("stripe", "mercadopago", "cash")
+ ALLOWED_PROVIDERS = ("stripe", "mercadopago", "cash", "klarpay")

  @post("/checkout")
  def checkout_endpoint(payload):
      if payload["provider"] not in ALLOWED_PROVIDERS:
          raise BadRequest(f"Proveedor no permitido: {payload['provider']}")
+     if payload["provider"] == "klarpay" and not settings.is_klarpay_enabled():
+         raise BadRequest("Klarpay no está disponible por el momento")
      order = build_order(payload)
      return checkout(order)

Archivo 5 — reports/reconciliation.py (nuevo, 140 líneas)

# Archivo: reports/reconciliation.py

PROVIDER_LABELS = {
    "stripe": "Tarjeta (Stripe)",
    "mercadopago": "MercadoPago",
    "cash": "Efectivo",
    "klarpay": "Klarpay",
}


class ReconciliationReport:
    def __init__(self, period, output_dir):
        self.period = period
        self.output_dir = output_dir

    def build(self):
        rows = db.fetch_reconciliation(self.period)
        rows = [{h: r[h] for h in RECON_HEADERS} for r in rows]
        rows = sorted(rows, key=lambda r: r["order_id"])
        body = ",".join(RECON_HEADERS) + "\n"
        for r in rows:
            body += ",".join(str(r[h]) for h in RECON_HEADERS) + "\n"
        path = f"{self.output_dir}/reconciliation_{self.period}.csv"
        write_text(path, body)
        return path

    def notify_finance(self, order):
        # Aviso a finanzas cuando una orden necesita revisión manual.
        subject = f"Revisión manual — orden #{order.id}"
        body = (
            f"Cliente: {order.customer_id}\n"
            f"Total: {order.total}\n"
            f"Proveedor: {PROVIDER_LABELS[order.provider]}\n"
            f"Estado: {order.status}\n"
            f"Creada: {order.created_at}\n"
            f"Boletos: {len(order.ticket_ids)}\n"
            f"Referencia: {order.external_ref}\n"
        )
        if order.provider == "cash" and order.status == "pending":
            body += "Pago en efectivo vencido.\n"
        organizer_email = order.customer.event.organizer.email
        mailer.send(FINANCE_EMAIL, subject, body)
        mailer.send(organizer_email, subject, body)

Archivo 6 — plugins/impls/klarpay_seating.py (nuevo)

# Archivo: plugins/impls/klarpay_seating.py
from plugins.base import SeatingPlugin
from plugins.registry import SeatingPluginRegistry


class KlarpayCheckoutExtension(SeatingPlugin):
    """
    No asigna asientos. Uso el registro de plugins porque ya existe
    el mecanismo y así el checkout puede llamar esto sin acoplarse.
    Manda el evento de Klarpay a analytics después de cada compra.
    """

    def assign(self, event, order):
        if order.provider == "klarpay":
            analytics.track("klarpay_charge", order_id=order.id, total=order.total)
        return None

    def release(self, seat): return True
    def is_available(self, event): return True
    def capacity(self, event): return 0
    def describe(self): return "klarpay-checkout-extension"


SeatingPluginRegistry.register("klarpay_checkout", KlarpayCheckoutExtension())

Y para contexto, así llama el checkout a los plugins (esto ya estaba en master):

# Archivo: checkout/checkout.py   (bloque 2, sin cambios en este PR)
    event = repository.get_event(tickets[0].event_id)
    if event.has_numbered_seats:
        plugin = SeatingPluginRegistry.get(settings.SEATING_PLUGIN)
        for ticket in tickets:
            ticket.seat = plugin.assign(event, order).code

Archivo 7 — tests/test_klarpay.py (nuevo)

# Archivo: tests/test_klarpay.py

def test_klarpay_charge_succeeds(monkeypatch):
    # Hay que resetear el singleton para poder inyectar la key de prueba.
    Settings._instance = None
    os.environ["KLARPAY_KEY"] = "test_key"
    os.environ["KLARPAY_MODE"] = "sandbox"

    monkeypatch.setattr(
        "klarpay_sdk.KlarpayClient.charge_card",
        lambda self, **kw: {"state": "OK", "tx": "kp_123"},
    )
    order = make_order(provider="klarpay", total=1000.0)
    checkout(order)
    assert order.status == "paid"


def test_registry_has_two_plugins():
    assert len(SeatingPluginRegistry.available()) == 2

Ejemplo trabajado: dos comentarios, hechos bien

Antes de que escribas los tuyos, vamos a hacer dos juntos. Elijo uno bloqueante y una nota, porque los dos tienen forma distinta.

Comentario A — bloqueante

El hallazgo: KlarpayProvider no implementa PaymentProvider. Tiene otros nombres de método (do_payment en vez de charge), otra firma (recibe amount y order_id en vez de order) y devuelve el dict crudo del SDK en vez de un ChargeResult.

La investigación primero, que es lo que la lección 2 pedía. Miro qué consecuencia tiene, no solo que sea distinto. Y encuentro tres cosas: el checkout tuvo que meter un if order.provider != "klarpay" para saltarse la llamada normal; tuvo que armar el ChargeResult a mano dentro del elif; y do_refund(payment_id) recibe algo distinto que refund(order), así que el flujo de reembolsos —que llama provider.refund(order)— va a fallar con Klarpay.

Esa tercera es la que convierte esto en bloqueante, y no se veía en el diff: hubo que ir a mirar quién más llama a refund.

Bloqueante — payments/klarpay_provider.py (todo el archivo).

KlarpayProvider no implementa la interfaz PaymentProvider que usan los otros tres: los métodos se llaman distinto (do_payment / do_refund en vez de charge / refund), reciben otra cosa (amount, order_id en vez de order) y devuelven el dict crudo del SDK en vez de un ChargeResult.

Tres consecuencias concretas, la tercera es la que bloquea:

1. El checkout tuvo que meter un if order.provider != "klarpay" (línea 58) para saltarse la llamada normal. Ese if va a tener que crecer con cada proveedor que no encaje.
2. La traducción del resultado quedó dentro del elif del checkout, así que el conocimiento del formato de Klarpay vive en el corazón del sistema en vez de en payments/.
3. refunds/service.py:44 llama provider.refund(order). Con Klarpay eso va a lanzar AttributeError en el primer reembolso, y va a pasar en producción porque no hay prueba de reembolso para este proveedor.

La dirección: que KlarpayProvider herede de PaymentProvider, reciba el cliente por constructor como los otros tres, y traduzca dentro:

class KlarpayProvider(PaymentProvider):
    def __init__(self, client):
        self.client = client

    def charge(self, order):
        # El SDK devuelve {"state": ..., "tx": ...}; aquí se traduce
        # al vocabulario del sistema para que el checkout no se entere.
        r = self.client.charge_card(value=order.total, ref=str(order.id), curr="MXN")
        return ChargeResult(ok=r["state"] == "OK", reference=r["tx"])

    def refund(self, order):
        return self.client.reverse(order.external_ref)

Con eso el elif del checkout vuelve a ser de dos líneas y el if order.provider != "klarpay" desaparece. Es lo que en el módulo 5 llamamos Adapter, y es exactamente lo que ya hacen StripeProvider y MercadoPagoProvider.

Comentario B — nota, no bloqueante

El hallazgo: el elif de Klarpay es el cuarto, y agregar un proveedor sigue tocando cinco archivos.

Aquí lo importante es quién es el culpable. Dani no creó este problema; lo heredó y lo padeció. Un comentario que la haga sentir responsable de un shotgun surgery que existía antes de que ella llegara es injusto y, además, no va a producir ningún cambio.

Nota, no bloqueante — el patrón de fondo, no este PR.

Este cambio dejó claro algo que ya venía: agregar un proveedor de pago es shotgun surgery. Un cambio conceptual —"cobramos con uno más"— tocó cinco lugares: checkout.py:41, config.py:18, api/routes.py:66, el archivo nuevo del proveedor y reports/reconciliation.py:3. Y el quinto va a repetir el recorrido.

El modo de falla que más preocupa es silencioso: si alguien agrega el elif y olvida PROVIDER_LABELS, el correo a finanzas revienta con KeyError dentro de un envío; si olvida ALLOWED_PROVIDERS, la API rechaza un proveedor que el checkout sí sabe cobrar y el error apunta al lugar equivocado.

Quiero ser explícita en una cosa: esto no lo introdujiste tú, es de la primera versión del sistema y hiciste lo único que se podía hacer con la estructura que hay. Lo dejo escrito porque este es el cuarto proveedor y ya tenemos evidencia suficiente para justificar el arreglo.

La dirección, para el ticket: un registro en payments/registry.py con un diccionario nombre → constructor más available() y LABELS, y que checkout, routes y reports lo consulten. El primer paso es chico y se mergea solo: extraer el if/elif actual tal cual a un payments.provider_for(name), sin cambiar nada más. Diez líneas movidas.

Abro el ticket y lo enlazo aquí. No hace falta que entre en este PR.

Qué esperar de estos dos. Fíjate en cuatro cosas.

La investigación cambió el peso. El primer hallazgo parecía "no sigue la convención" —una sugerencia— hasta que fui a ver quién llama a refund. El bug de reembolsos es lo que lo hace bloqueante, y no estaba en el diff. Ese paso de ir a mirar afuera es el que separa una revisión de una lectura.

El bloqueante trae el código. No es obligatorio, pero cuando la dirección cabe en diez líneas, escribirlas ahorra una ida y vuelta completa y baja muchísimo el costo percibido de aceptar.

La nota dice de quién es el problema. La frase "esto no lo introdujiste tú" no es cortesía: es información que cambia lo que Dani tiene que hacer. Sin ella, un comentario largo sobre shotgun surgery se lee como un reproche por algo que ella no eligió.

Los dos tienen peso desde la primera línea. Con eso, Dani sabe en cinco segundos cuál de los dos afecta su viernes.

Lo que hay que entregar

Un solo documento —o el conjunto de comentarios en la herramienta que uses— con tres partes.

Parte 1 — Los comentarios de línea (entre 8 y 12)

Cada uno con las cinco partes de la lección 4:

• Peso — qué esperas del autor: si bloquea la aprobación, si es una propuesta que puede declinar, o si es una nota que se deja escrita para después. (Las etiquetas formales —bloqueo, sugerencia, nit— y el criterio para elegirlas están en clean-code-and-code-review-guide, módulo 4, lección 5.)
• Ubicación — archivo y línea, o la lista completa si el problema es de dispersión. Si está fuera del diff, dilo.
• Nombre — el olor o anti-patrón, con su glosa de media línea. Si no estás seguro del nombre, describe la estructura en vez de arriesgar uno equivocado.
• Consecuencia — qué va a doler, cuándo, o qué se rompe si se olvida algo. En términos de trabajo futuro o riesgo, no de principios.
• Dirección — una opción concreta, con estimación de tamaño. Si excede el PR, dilo y propón ticket.

Hay al menos nueve problemas distintos en este PR. No hace falta que los encuentres todos; sí hace falta que los que encuentres estén bien escritos. Y presta atención al reparto de pesos: si todo te sale bloqueante, revisa el criterio.

Parte 2 — El comentario general del PR

Cinco o seis líneas al principio de la revisión, que respondan tres cosas:

• Qué hace este PR y qué reconoces de bueno en él. Concreto, no de cortesía.
• Cuáles son los dos o tres puntos que de verdad importan, para que se lean primero.
• Qué esperas que pase antes de aprobar, con la fecha del viernes sobre la mesa.

Parte 3 — El veredicto justificado

Elige uno —aprobar, aprobar con comentarios, solicitar cambios o comentar sin decidir— y justifícalo en tres o cuatro líneas. Tiene que ser coherente con los pesos que asignaste: si marcaste tres bloqueantes, no puedes aprobar.

Y una restricción que es parte del ejercicio: el viernes existe. Una revisión que ignora que hay una fecha comercial real no es una buena revisión, es una revisión en el vacío. Si tu veredicto implica que el PR no entra esta semana, di qué propones hacer con eso.

Cómo se evalúa

La prueba principal, aplicada comentario por comentario: ¿podría Dani actuar sobre esto sin volver a preguntar?

Léelo desde su silla. Ella tiene tu comentario abierto, tú estás en una reunión, es lunes a las nueve. Para cada comentario, ¿sabe dónde mirar, qué está mal, por qué importa, qué hacer y si tiene que hacerlo hoy? Si a alguno de esos cinco tiene que contestarle "depende" o "habría que preguntarle", ese comentario falló.

Cuatro señales de que la revisión está bien hecha:

Los pesos están repartidos. Una revisión donde todo es bloqueante no distingue una interfaz rota de un timeout raro, y produce el mismo efecto que una donde nada lo es: el autor decide solo qué importa.

Se distingue lo introducido de lo heredado. Al menos tres de los nueve problemas de este PR existían antes. Una revisión que se los cobra a Dani está mal, aunque los diagnostique correctamente.

Las consecuencias son verificables. Cada una debería poder confirmarse o refutarse abriendo un archivo. Si tus consecuencias son "esto no escala" o "viola SRP", no hay nada que verificar.

Las direcciones tienen primer paso. "Hay que refactorizar el manejo de proveedores" no es una dirección. "Extraer el if/elif tal cual a payments.provider_for(name), diez líneas, se mergea solo" sí lo es.

Y dos formas de fallar que conviene nombrar: la revisión que enumera sin priorizar —correcta y agotadora, produce que se arregle lo barato y quede lo caro— y la revisión que rediseña el sistema en el margen —donde el PR de una funcionalidad se convierte en un rediseño y se atasca dos semanas—.

Errores comunes

Cobrarle a Dani lo que heredó (de criterio y de trato). Qué pasa: la revisión diagnostica correctamente el God object de checkout.py, el Singleton de config.py y el registro de plugins, y los escribe como si Dani los hubiera introducido. Ella lee cuatro comentarios sobre decisiones que no tomó, en su primer cambio grande sobre el sistema, tres semanas antes de un festival. El efecto previsible: se desmoraliza, arregla lo que puede y aprende que tocar el checkout es caro. Por qué pasa: el PR es el momento en que esos problemas se hacen visibles, y la visibilidad se confunde con la autoría. Cómo detectarlo: para cada hallazgo, pregúntate si existía en master antes de este PR. Si existía, no es de ella. Cómo corregirlo: dilo explícitamente —"esto no lo introdujiste tú"— y sepáralo de lo que sí es suyo. Lo que sí es de Dani en este PR: la interfaz divergente del proveedor, la comisión metida en el checkout, el INSERT en el checkout, el uso del registro de plugins para algo que no son asientos, y la cadena order.customer.event.organizer.email. Eso es bastante y alcanza; no hace falta agregarle el pasado del sistema.

Bloquear por estructura y perder la fecha (de proceso). Qué pasa: la revisión marca como bloqueantes el shotgun surgery, el God object y el Singleton —los tres diagnósticos correctos— y el PR no entra el viernes. Klarpay no se prueba en staging, el festival llega, y alguien acaba mergeando a las apuradas sin revisión. La estructura no mejoró y encima se perdió el control de calidad. Por qué pasa: un problema real se siente como razón suficiente para bloquear, y el vocabulario nuevo da confianza para hacerlo. Cómo detectarlo: si tus bloqueantes suman más trabajo que el PR original, algo está mal calibrado. Cómo corregirlo: bloquea solo lo que rompe algo o lo que va a ser mucho más caro de arreglar después. En este PR, eso son la interfaz divergente —por el bug de reembolsos—, el plugin mal ubicado —porque se ejecuta en cada compra con asientos numerados— y probablemente el INSERT sin transacción. Todo lo demás se documenta con evidencia completa y se marca como nota. El estándar publicado de Google lo dice mejor que yo: se aprueba cuando el cambio mejora la salud del sistema, no cuando lo deja perfecto.

Escribir la revisión sin salir del diff (de método). Qué pasa: la revisión analiza con cuidado las líneas que cambiaron y no encuentra lo más grave, porque lo más grave de este PR no está en el diff. El bug de reembolsos aparece en refunds/service.py, que el PR ni toca. El plugin de Klarpay es peligroso por cómo lo llama checkout.py:31, que tampoco cambió. Y PROVIDER_LABELS es un problema por lo que pasa cuando falta una clave, no por la clave que Dani agregó. Por qué pasa: la herramienta muestra el diff, y el diff se siente como el alcance del trabajo. Cómo detectarlo: si tu revisión no cita ni un archivo fuera del diff, probablemente no saliste. Cómo corregirlo: por cada símbolo que el PR introduce o cambia, un grep de quién más lo usa. Tres búsquedas alcanzan para este PR: quién llama a refund, quién itera el registro de plugins, y quién más lee PROVIDER_LABELS. Cinco minutos, y son los que producen los dos mejores comentarios de la revisión.

Ejercicios

Ejercicio 1 — Escribe el comentario general del PR. Antes de los comentarios de línea, escribe la parte 2 del entregable: cinco o seis líneas con lo que reconoces, los dos o tres puntos que importan, y qué esperas antes de aprobar, con el viernes sobre la mesa.

Ejercicio 2 — Diagnostica los tres problemas escondidos. Tres de los problemas de este PR no se ven leyendo solo el diff. Encuéntralos, di qué búsqueda los revela y escribe el comentario de uno de los tres.

plugin = SeatingPluginRegistry.get(settings.SEATING_PLUGIN)
for ticket in tickets:
    ticket.seat = plugin.assign(event, order).code

Ejercicio 3 — Escribe el veredicto y defiéndelo. Con tus nueve comentarios sobre la mesa, escribe la parte 3: el veredicto y su justificación en tres o cuatro líneas. Después responde: si Dani te contesta "los tres bloqueantes los arreglo, pero el del plugin lo necesito porque no encontré otra forma de que el checkout mande el evento", ¿qué respondes?

Resumen y siguiente paso

En este proyecto hiciste lo que promete la guía: recibiste un pull request real sobre Boletia y escribiste la revisión completa, sin tocar una línea de código. Nueve problemas de gravedad muy distinta, repartidos entre los que Dani introdujo —la interfaz divergente, la comisión y el INSERT en el checkout, el plugin mal ubicado, la cadena order.customer.event.organizer.email— y los que heredó del sistema —el if/elif de proveedores, el God object del checkout, el Singleton de config.py, el esqueleto duplicado de los exportadores—.

Aplicaste las siete lecciones juntas: reconociste los olores (2), usaste la detección contable de los tres grandes (3), escribiste cada comentario con sus cinco partes (4), nombraste los anti-patrones sin cobrárselos a quien no los eligió (5), diste direcciones con primer paso mergeable (6) y elegiste el registro de cada comentario según tu grado de certeza (7).

Y practicaste tres cosas que no son vocabulario y deciden si una revisión sirve: salir del diff —los tres problemas más graves aparecían con un grep, no leyendo las líneas cambiadas—, repartir los pesos —tres bloqueantes de nueve hallazgos, para que la jerarquía viaje con el texto— y hacerte cargo de la fecha, porque una revisión que ignora que el festival es en tres semanas no es rigurosa, es irreal.

La prueba con la que se juzga sigue siendo la misma: si Dani podría actuar sobre cada comentario sin volver a preguntar. Léela una vez más desde su silla antes de dar por terminada tu entrega.

Con esto cierra el módulo 7 y cierra el vocabulario de la guía. Tienes las dos mitades del diccionario: los nombres de lo que funciona —Strategy, Factory, Adapter, Observer— y los de lo que falla —God object, feature envy, shotgun surgery, los anti-patrones—. Y tienes la forma de usarlos con otra persona.

Lo que queda es la última pieza y es la más difícil: hacerlo. El módulo 8 es el capstone, y ahí dejas de comentar el trabajo de otro para tomar el tuyo. Vas a recibir dos rincones de Boletia —uno sobre-patronado, plugins/, y uno sub-estructurado, el checkout de trescientas líneas— y sobre uno vas a quitar la abstracción que no se gana su lugar y sobre el otro vas a introducir el patrón que de verdad emerge del problema, en pasos pequeños y seguros. Y vas a entregar, además del código, el documento que justifica cada decisión con su tradeoff. Se juzga por el criterio y la comunicación, no por la cantidad de patrones aplicados — que es, en el fondo, lo mismo que se juzgó aquí.

Recursos

• Google Engineering Practices — The Standard of Code Review — el criterio que decide un veredicto: aprobar cuando el cambio mejora la salud del sistema, no cuando lo deja perfecto. Léelo antes de escribir tu parte 3.
• Conventional Comments — la convención para marcar el peso de cada comentario. Si tu equipo no tiene una, esta sirve tal cual y se adopta en quince minutos.
• How to Do Code Reviews Like a Human (Michael Lynch) — ejemplos concretos de reescritura de comentarios, con especial atención a cómo se dice algo grave sin que se lea como un reproche.
• Refactoring Guru — Code Smells — el catálogo de consulta mientras escribes la revisión: para cada olor, las refactorizaciones asociadas de donde salen las direcciones que propones.