“Érase una vez una revisión de código, escribiendo comentarios por correo, indicando el número de línea. Fue muy gracioso. De los pros: nadie miró nada en las diferencias, miró en el IDE. Pero también hubo un inconveniente: después de algunas fusiones, los números de línea cambiaron ".
Alexander Makarov, Yii
“Nuestra empresa tiene un concepto interesante: una solicitud de silla . Esto es cuando, dentro de la misma oficina, un desarrollador se acerca a ti en una silla y te dice: "Mira, esto es más rápido que crear una solicitud de extracción".
Anton Morev, WormSoft
Recientemente, hubo una grabación pública del podcast SDCast sobre revisión de código en YouTube. Hemos seleccionado y descifrado lo más interesante del tema.
Versión de audio completa en Spotify, Ya Music y como archivo para descargar
Versión de video completa en Youtube
No trate las revisiones de código como si estuviera comprobando algo o buscando errores
Sergey Zhuk, Skyeng: Creo que el objetivo fundamental de una revisión de código es compartir conocimientos dentro del equipo y encontrar la mejor solución. El equipo revisa los cambios propuestos y el conocimiento sobre el dominio se distribuye uniformemente entre sus miembros. El autor de la solución comprende cómo se puede mejorar el código que pensó que era perfecto.
Por lo tanto, las revisiones de código no son lo que se debe evitar o hacer más rápido. Esta es una inversión en el negocio y el equipo: el código mejora, el equipo mejora. Sí, al principio no me gustó cuando se envolvió la solicitud (ya quién le gustaría eso).
Pero luego comencé a verlo como un proceso de aprendizaje, como leer libros o participar en conferencias.
Sentí esto más yo mismo, como desarrollador crecí con este enfoque.
Pero hay un matiz. Seguramente muchos de ustedes se han encontrado con solicitudes de mil líneas, donde se mezclaron la refactorización, una característica y algunos cambios menores. Al mezclar diferentes cosas en una solicitud, complicamos tanto el intercambio de conocimientos como la vida del revisor: le resultará más difícil evaluar si el comportamiento del sistema ha cambiado, si se ha cumplido el requisito empresarial, si el problema en su conjunto se ha resuelto y si la solución es satisfactoria.
Notamos este momento en nuestro equipo e introdujimos una regla: en una solicitud de extracción, no interfiera con cambios de otra naturaleza. Se envía la refactorización por separado, la función por separado y los pequeños cambios por separado.
Informe de Sergey sobre las prácticas de su equipo. La versión de texto está aquí .
Por lo general, estas solicitudes se revisan rápida y fácilmente, y es mucho más probable que reciban comentarios de calidad. Y por parte del mantenedor, hay ventajas: si le gusta la refactorización y la función con un error, puede fusionar la refactorización por separado.
Alexander Makarov: Estoy de acuerdo en que no debería intentar dedicar el menor tiempo posible a las revisiones de código. Abierto, como si los corchetes estuvieran bien, este código hace algo, no funciona de esa manera. Si el revisor no puede garantizar el código hasta el final, significa que la revisión del código no se ha llevado a cabo.
Por lo tanto, ahora hemos comenzado a compilar una gran cantidad de pautas para nosotros, y una de ellas habla sobre la revisión del código.
Parte de la guía del equipo de Yii.
Pero a la tesis sobre las pequeñas solicitudes de extracción separadas: tuve situaciones en las que llegó un cliente potencial e introdujo algo así. El equipo fue hostil. Cómo lo conseguiste?
Sergey Zhuk: Había un equipo en paralelo con el que interactuamos y usamos su API. Hicieron un montón de funciones en un mes, nosotros solo hicimos un poco. Es decir, inicialmente no nos centramos en la entrega de funciones, sino en la calidad con este enfoque. Y acordamos con el negocio que lo soltaremos más lentamente, pero tratamos de no romper nada. Un mes después, uno de los vecinos se derrumbó, luego otro. Pero no lo hacemos. Y en este ejemplo, todos entendieron todo.
Konstantin Burkalev, SDCast:Tuve procesos de implementación de revisión de código en general, y no fue fácil, aunque todos parecían entender que era bueno, sí. Dices: "Chicos, fusionemos mediante una solicitud de extracción". Te dicen: "Sí, hay una pequeña característica".
El principio aquí realmente funciona bien: cuando algo se rompe, dices: "Pero si hiciste una solicitud, habríamos mirado y simplemente no llegamos a eso". La idea es que las personas comprendan los errores a partir de su propia experiencia. Tratar de imponerse no siempre funciona.
Qué tan rápido revisar
Durante la transmisión, se realizó una votación entre la audiencia. Aqui esta uno de ellos.
Konstantin Burkalev: Junio es especialmente a menudo así: “Bueno, miraste mi solicitud, ¿está bien ahí? Lo escribí, apreté los puños y esperé ”.
Y tengo mi propia tarea, solo puedo llegar por la noche o no sé nada ...
Sergey Zhuk: En nuestro equipo, la revisión siempre ha sido una prioridad. Por lo tanto, hay una regulación: cuando llega una solicitud de extracción, terminas lo que estás haciendo ahora, para no perder el contexto, y vas a revisar. Porque lo que estás a punto de hacer aún está en proceso. Y esa tarea ya está hecha.
El código ya está escrito, solo queda buscarlo, fusionarlo y subirlo al producto.
Es decir, terminé algo propio, cambié, miré rápidamente y seguí trabajando. Probablemente, 3 veces al día me distraigo de mis tareas para revisarlas. Pero, nuevamente, debes entender que en mi equipo todo está dividido en pequeñas solicitudes de extracción, de 200 a 300 líneas de código cada una. En consecuencia, se gasta un mínimo de tiempo.
"Quién reseña es menos importante que quién"
Konstantin Burkalev: Esto plantea la pregunta: quién debería revisar. ¿Solo plomo? ¿Solo el señor? ¿O puede y debe darse a alguien de menor competencia, solo para que una persona intente crecer?
Y cuando se les preguntó qué revisar, la gente respondió así.
Alexander Makarov: Si tienes mucha gente en tu equipo, y el líder ha creado un cuello de botella en sí mismo, ralentiza a todo el equipo y, como resultado, lo empeora mucho. Como líder, cuando trabajaba en Skyeng, tenía 15 solicitudes de extracción por día en su punto máximo, y no las más pequeñas. Dejo tiempo para la revisión por la mañana y por la noche.
Es necesario revisar a todos. Excepto, quizás, las historias en las que "fuego, todo está en llamas", no empeorará allí.
Me equivoco, esto es normal, aunque he estado usando el mismo proyecto durante muchos, muchos años. Por lo tanto, ahora intento invitar al máximo número de personas a ver mi solicitud. Esto es bueno y debería serlo.
Otra cuestión es si todos deben confiar en la revisión. Tenía muchachos que podían hacerlo bien, pero tenían grandes problemas para concentrarse: y, digamos, uno dedicó 6 horas a una revisión. Tuve que enseñarle a la gente cómo administrar su tiempo.
Si hablamos de empresas comerciales, estoy a favor de especificar quién tiene esta responsabilidad y quién puede revisar a voluntad, y cuánto tiempo por día puede dedicar a la revisión, dependiendo de este estado.
Anton Morev:Como veo, hay diferentes procesos. Si estamos haciendo alguna característica que necesita ser lanzada en poco tiempo, no tenemos tiempo para dejar que Jun vea el código. Pero si estamos haciendo algún tipo de producto interno o la fecha límite no es alta, sí, es una buena práctica dejar que June revise lo que hizo el desarrollador principal o líder. Por lo tanto, los Juns comprenderán mejor lo que está sucediendo en el proyecto en su conjunto y cómo funciona el mecanismo de toma de decisiones.
"Este es un rechazo de inmediato"
Sergey Zhuk: Skyeng implementó una verificación para un mensaje de confirmación: debe haber un número de tarea en JIRA; de lo contrario, la solicitud de extracción no se puede combinar. A veces, abres el código, simplemente no entiendes lo que hay allí, abres una tarea y puedes entender algo.
Por lo demás, al principio fue difícil para nosotros, luego decidimos rechazarlo solo si la tarea era completamente incorrecta o el equipo no estaba de acuerdo arquitectónicamente. Y entonces: ponga una aprobación, fusione, escriba un comentario, y si el autor de la solicitud de extracción quiere, regresará y lo arreglará. Allí, corregirá pequeñas asperezas o utilizará algún tipo de patrón. ¿Cuáles son sus prácticas de rechazo?
Alexander Makarov: Los criterios coinciden completamente con los suyos: si la tarea no se hace bien, por supuesto, debe terminar. Incluso si el código se ve bien y arquitectónicamente todo está bien.
, : , .
Por ejemplo, decimos: "Chicos, hagamos una prueba". Hay excepciones, por supuesto, pero las pruebas son muy importantes. De ellos queda claro si la persona entendió la tarea correctamente: nuevamente, si prueba clases y métodos, y no un caso de uso, esto ya es sospechoso. Ahora hemos estropeado la infección , esto es una prueba de mutación. Tulza sale de las mismas pruebas, pero modifica el código y se ejecuta. Y si el código modificado pasó con las mismas pruebas, entonces parte del código no es necesaria; simplemente puede tomarlo, eliminarlo, nada cambiará.
Un poco entre bastidores.
Además, por supuesto, problemas de seguridad y rendimiento: aquí habrá un rechazo. No rechazamos las nimiedades: o pedimos que las arreglemos o las arreglamos nosotros mismos; presionamos directamente en las solicitudes de extracción de quienes las hicieron, y ya mostramos en el código terminado qué, cómo y por qué lo hicimos.
Anton Morev:Respecto a lo que dijiste, ¿está resuelto el problema? Sucede que un desarrollador, mientras resuelve un problema, ha resuelto otro. No es necesario rechazar tales situaciones. O al menos averiguar cómo se moderó la tarea.
Konstantin Burkalev: Pero el momento de vincular los mensajes de confirmación con un rastreador de tareas me parece importante. Hay tareas diarias en las que ayuda mucho. ¿Sabes cuándo? "Escucha, hicimos algo similar en el problema del botón". Encuentra la tarea sobre el botón, comprende el número, va al registro del repositorio, encuentra la diferencia de esas confirmaciones y ve: de hecho, hemos estropeado esto y aquello, se puede mover aquí.
Pero también ocurre lo contrario. Estoy mirando el código fuente de un proyecto aquí y me encuentro con la función action684 en el backend.
Le escribo a un amigo y le pregunto cuál es este nombre genial en el código. Él responde: una referencia a la tarea en el rastreador. "¿Por qué inventar un nombre para una función? Lo estás escribiendo como parte de una tarea" ... Thresh, que no debería existir en un mundo normal, por supuesto.