Code Review
Fecha: June 5, 2019
Transcripción De: Bryan Bishop
Traducción Por: Blue Moon
Categoría: Core dev tech
Encuesta de revisión de códigos y reclamaciones
https://twitter.com/kanzure/status/1136261311359324162
Introducción
Quería hablar sobre el proceso de revisión de código para Bitcoin Core. No he hecho revisiones de código, pero siguiendo el proyecto durante el último año he oído que este es un punto de dolor para el proyecto y creo que a la mayoría de los desarrolladores les encantaría verlo mejorado. Me gustaría ayudar de alguna manera para ayudar a infundir un poco de energía para ayudar con las revisiones de código. Envié una encuesta. 11 personas respondieron. Voy a resumir los resultados de eso, entonces me gustaría pasar nuestro tiempo hablando de ideas sobre cómo mejorar algunas de las áreas del proceso. Tengo algunos experimentos concretos que podrían tener sentido, pero quiero obtener retroalimentación de la gente y necesitaré algunos voluntarios.
Resultados de la encuesta
11 personas respondieron, y todas ellas se mostraron abiertas a la experimentación. Todos menos 2 de los encuestados son desarrolladores que quieren hacer más revisiones de código de las que hacen actualmente. Creo que hay interés en hacer más. Puede que haya un componente de culpabilidad. ¿Deberíamos trabajar para aumentar la culpabilidad? Deberíamos aprovecharlo.
Una de las preguntas era: ¿cómo se selecciona un RP para revisarlo y cómo se revisa? No parece haber un proceso consensuado claro para hacerlo. Podría ser un área de mejora. En general, la gente estaba satisfecha con la calidad de la revisión del código, pero no demasiado. Creo que la mayoría deseaba mejorar la coordinación de la revisión de los RP.
¿Cómo decidimos qué revisar? ¿Qué fomentaría un mayor número de revisiones? Tres cosas que obtuvieron muchas respuestas en la encuesta fueron: para fomentar las revisiones de los RP, una fue identificar los RP para las revisiones, y los autores de los RP declarando una mejor motivación para el RP, y más pruebas junto con los RP. Todas ellas obtuvieron entre 5 y 7 respuestas de las 11.
Por lo general, los desarrolladores no saben cuántas horas dedican a la revisión. Las revisiones de una hora son muy diferentes de las de 5 minutos. Tal vez deberíamos pedir a la gente que haga un seguimiento de esto. ¿Sería valioso hacer un seguimiento de las métricas de las revisiones de código? Tal vez tres personas dijeron que sí, pero la mayoría pensó que no, que no era un retorno de la inversión lo suficientemente alto.
Podríamos mantener una lista de las personas que encontraron un error en los PRs de otra persona. Git tiene soporte para review-by y ACK-by. Es una convención, no una característica. El proyecto linux kernel lo hace, por ejemplo. Es texto libre que las herramientas reconocen.
Valdría la pena acreditar a las personas en las versiones por el trabajo de revisión, incluso si no escribieron el código ellos mismos. Esto se puede extraer de la API de github.
Tenemos la alta prioridad para la lista de revisión. No tengo la impresión de que este sistema funcione muy bien.
Experimentos
Tengo 5-6 experimentos que podría revisar.
Hay como 5-6 PRs en la lista de alta prioridad. Podría ofrecerme voluntario para ayudar a coordinar las revisiones de los mismos trabajando con el autor del RP para identificar quiénes son los revisores candidatos para el RP y luego salir y ver si esas personas revisarán el RP y darles un empujón y tratar de acordar horarios. ¿Creemos que podemos hacerlo en el plazo de una semana o en algún otro plazo? Tendría que preguntarles a ustedes, ¿qué es razonable en términos de duración?
Creo que en general sería bueno si hubiera una convención social en la que está bien empujar a alguien como «hey quiero que revises eso» o alguien coordinando eso. No podemos dejar que los autores elijan a los únicos revisores, pero pueden darles un empujoncito. Algunas personas no se sienten cómodas pidiendo una reseña, así que tener un coordinador puede ayudar en este sentido. No es sólo una bolsa de revisiones, es utilizar los conocimientos para coordinar las peticiones de revisión. Si hay una persona que coordina, podrá retrasar las revisiones de ciertas cosas si alguien está sobrecargado, ya que el coordinador tiene una mejor visión de lo que la gente está mirando.
¿Cuándo dispondrán los revisores del tiempo necesario para la revisión? Eso ayudará a establecer expectativas sobre el calendario. Esto ayuda a todo el mundo a planificar.
Si tener un coordinador funciona, me gustaría codificarlo para que pudiera escalar mejor. Quizá sea una actividad de gestor de proyectos. En la primera fase, lo haría para aprender.
Una de las cosas que aprendería es averiguar qué partes del código tienen que ir a quién. Github tiene soporte para archivos MAINTAINERS. Desafortunadamente nuestra estructura de directorios no es directamente compatible con eso.
Me gustaría que se hiciera más hincapié en los Concept ACK. Los ACK de concepto pueden ayudar a determinar las prioridades de revisión. A veces es el problema contrario. A menudo nos falta un Concept NACK inicial, y luego hacen falta unas cuantas cosas para que alguien venga y muestre que no está de acuerdo. La lista de alta prioridad debe ser generalmente concepto ACKed. Nada debería entrar en la lista si no es un concepto ACK. ((Algún desacuerdo aquí.)) Anteriormente la política era «todo el mundo tiene una cosa en la lista» y estar en la lista no es un juicio ni nada, pero sería un buen uso del tiempo de la reunión para convertir eso en un … cada persona llega a nominar algo (no necesariamente su propio PR). Como mínimo debería haber algún juicio, y si no es realmente prioritario debería ser al menos un concepto ACK, es ridículo tener algo ahí que no queremos que se fusione. Pero a veces dar un ACK de concepto no es trivial, y requiere discusión en algún momento. Si encuentras un PR que no tiene comentarios y crees que es interesante, entonces di «creo que esto podría ser interesante» al menos.
Yo me centraría en los RRPP obsoletos y, o bien conseguiría que los rechazaran, o bien conseguiría que los revisaran. Conseguir ACKs de concepto es una forma de conseguir que el autor de un PR se sienta más animado.
A veces me entero de PRs semanas más tarde. ¿Usar github assignment y asignarlos a un PR? Se pueden asignar a no mantenedores.
En nuestro flujo de trabajo, es algo inusual que generalmente requiramos múltiples revisiones de múltiples personas. No se puede asignar la revisión a una sola persona, porque una sola persona no es suficiente. Pero asignar a una persona que es- cuando se le asigna, es su trabajo para mantener el proceso de revisión o algo así. Es el trabajo de los autores para fusionar- especialmente para la lista de alta prioridad. Para los nuevos contribuyentes es diferente.
En el kernel linux, hay mantenedores de subsistemas y especialistas que entienden diferentes partes y mucho más interesados en esos subsistemas. Hay formas de que otros desarrolladores se apunten para prestar atención a ese tipo de problemas. Me pregunto si ayudaría que la gente se ofreciera voluntaria. Ya hay etiquetas en los problemas de github añadidas por fanquake en cuestión de minutos.
El propósito original de alta prioridad para la revisión, fue originalmente acerca de las cosas que están bloqueando a la gente. Esto fue un gran motivador. Hay que motivar al RP para la inclusión en la lista de alta prioridad para revisión. La página de alta prioridad debe tener algún texto o descripción, como quién lo propuso y por qué. Lo ideal sería hacerlo durante la reunión, pero tener descripciones podría ayudar en alguna página.
La lista de alta prioridad para revisión a veces es realmente difícil para los revisores, son RP difíciles. Es normal. He oído ese problema de otras personas. Existe una tendencia natural a revisar los RP más pequeños porque puedes eliminarlos rápidamente. Las de mayor impacto se retrasan más de lo debido o nunca se revisan. ¿Cómo podemos mejorar esta situación? Una idea es que nadie pueda tener más de 5 relaciones públicas abiertas al mismo tiempo. Eso reduciría las RRPP abiertas a 100 o 150 RRPP. Esto limitaría el trabajo. Esto podría animar a la gente a hacer PRs más grandes. Pero deberían dividirlo y centrarse en una cosa a la vez. Pero en la práctica envían múltiples PRs. Cuando un bot cierra automáticamente mi RP y dice lo siento tienes demasiados RP abiertos, mi respuesta no va a ser «meh» sino que voy a abandonarlo. Pero parte del problema es que estás obligando a todo el mundo a hacer una gestión de prioridades. Como revisor, si veo a alguien con una secuencia de múltiples PRs, diré, oh este incluye este otro, solo miraré el primero.
Los PR importantes no reciben revisiones porque son demasiado complicados de entender. El límite de 5 PR no aborda esto de ninguna manera. ¿Qué lo solucionaría? Relaciones públicas más pequeñas, tal vez. Deberías poder comentar parcialmente un RP y volver a él más tarde. En Facebook, cuando hacen un PR, lo revisan commit por commit, y cada commit que se revisa se fusiona inmediatamente. Pero esto podría no funcionar para nosotros, como refactorizar commits si el resultado final no va a ir a ninguna parte. Si lo haces commit por commit, los PRs se hacen más pequeños, y consigues que las cosas se fusionen.
Sobre el límite de 5 PRs, ahora mismo tengo PRs abiertos y ahora mismo no tengo una razón para empujar a la gente. Pero si tengo un límite, tendré que empezar a presionar a la gente. Sobre el concepto ACK vs nit, no he pensado en ello, voy a pensarlo, pero no voy a escribir un montón de nits.
¿Queremos hacer que en la reunión se decida la alta prioridad de las revisiones sean ACKs de concepto automático? ¿Como si tuvieras derecho durante la reunión a defender que algo debería estar en la lista? Algunas personas no pueden estar presentes en la reunión, así que no puedes hacer que debas defenderlo durante la reunión. El autor debe presentar un caso claro en el RP. En la reunión, pueden hacer referencia al RP y luego tú haces clic en él y ves la motivación. La persona que lo pone en la lista de alta prioridad debe verificar que ha recibido ACKs de concepto o decir que necesita ACKs de concepto. Decidir qué va en la lista de alta prioridad, si es «todo el mundo puede nominar uno», entonces no necesitamos una reunión para eso. Me parece un uso útil del tiempo de la reunión utilizarla para hacer ACK de concepto. Pero está la cuestión de que no todo el mundo puede estar presente todo el tiempo. No debería haber decisiones finales en la reunión, pero discutir cosas está bien. Las correcciones de errores antes de un lanzamiento, deben ser automáticamente concepto ACKed. Tal vez en github, tener subproyectos de alta prioridad.
¿Qué tal si, en lugar de un límite de 5 PR, el autor puede etiquetar un nivel de prioridad? Pero esto no funcionó en el pasado. ¿Cómo deciden qué es de alta prioridad? Como autor de un RP, una de las cosas que se preguntaban en la encuesta era si varias cosas del RP te animarían a revisarlo más. Mejor motivación. Proporcionar pruebas. Estas son dos cosas que obtuvieron buenas respuestas en la encuesta. Otras dos que mencioné pero que no obtuvieron votos fueron incluir un documento de diseño y una guía sobre cómo revisar este RP. Podemos debatir las ventajas de cada una de ellas. Esto haría que algunos RP fueran más revisables. Como autor de un RP, si crees que uno de ellos es el más importante o prioritario, ponte a trabajar para hacer estas cosas extra y eso indicará que es prioritario, porque habrás hecho que sea más fácil de revisar. Como proyecto, podríamos tener eso como un criterio para estar en la lista de alta prioridad, o podríamos tener otra lista que sea una lista de PR de calidad vetada, básicamente aquellos que han sido vetados, que tienen una buena motivación, que tienen pruebas, que tienen un documento de diseño, etc. Esto también podría mejorar la visibilidad de los PRs si están buscando PRs para revisar y saben que en esa lista el autor se está preocupando de hacerlos lo más revisables posible.
¿Queremos pensar en ACKs de concepto y ACKs de revisión de código como etapas separadas? Esto es algo que me molesta personalmente, cuando como, una idea vaga altamente experimental PR y se entierra bajo nits código. Ya no quieres ni entrar a hablar del concepto. No sé si más estructura es la solución para esto. No se hace una revisión real del código hasta que …. no está seguro de que esto se pueda evitar. La integración de rust en Bitcoin Core PR fue como algo muy experimental, y el primer comentario fue NACK. Pero incluso si se nos ocurre este sistema de etiquetas, me temo que podría no funcionar. ¿Discusión de conceptos en issues? Suhas lo ha intentado. Tal vez una etiqueta necesita-concepto-ACK. Tal vez el autor puede solicitar que este concepto necesita ACK, y uno de los mantenedores añadiría esta etiqueta. Esto significa que la discusión debe limitarse a las ideas de alto nivel. ¿Es posible añadir temas a la lista de alta prioridad para la revisión, que podría ser una buena manera de tener una discusión de conceptos. Podríamos pedirle al CEO de github que haga imposible hacer comentarios de línea, porque hace el hilo ilegible, porque el resto de los revisores estaban mirando nits de código. El desorden podría ser un buen comentario para el CEO de github. Recibes un correo electrónico cada vez que alguien comenta, y puede que ya no veas la discusión del concepto. Tal vez una casilla de verificación para «esto es una liendre» y luego no activar la notificación por correo electrónico para eso. A continuación, podría configurar, no recibir notificaciones de liendres, o filtrarlos en la visibilidad. A veces no quieres comentar porque va a volar la bandeja de entrada de alguien. Así que es a la vez el desorden de notificación, y también la interfaz web se desordena.
Como autor, deberías añadir a alguien a la revisión, si arregla algo significativo. El autor debe asumir la responsabilidad de añadir a alguien como commit, o añadir un «gracias a fulanito». Github soporta coautores ahora. No quieres darles la capacidad de empujar, pero tal vez deberíamos hablar de ellos reconociendo a los autores de código. Elijas a quien elijas, es cuestión de criterio, no hagas spam. En el pasado, nos hemos acordado de volver atrás y dar crédito a alguien por informar de un error. Podríamos ser más considerados a la hora de hacerlo durante el proceso de revisión, y no sólo fuera de los informes de errores.
Muchas de las revisiones son más puntillosas que una reflexión profunda sobre lo que se pretende conseguir o más holística. ¿Cómo podríamos fomentar revisiones de mayor calidad? El autor podría ofrecer una guía para la revisión. Pero esto no consigue lo que quiere. Es como una lista de comprobación de lo que se ha revisado, pero lo ideal es que el revisor elabore su propio plan de revisión para averiguar cuáles son los problemas. Cuando escribes un tACK o algo así, deberías escribir tu propia revisión de los pasos que has dado, o de los pasos que recomiendas. Cuando alguien hace un ACK, ¿qué ha hecho realmente? ¿Le dedicaron 5 minutos o 5 horas? Quizás reemplazar los ACKs con explicaciones, como «He comprobado estos hashes» o «ACK, he verificado las claves gpg». Tal vez deberíamos deshacernos del simple ACK e intentar obtener alguna explicación. Además, deberías incluir el ID del commit con el mensaje ACK. Tal vez un botón en la interfaz para ACK un commit específico. Pero esto podría llevar a más ACKs espurios.
¿Cuántos de ustedes están revisando en github versus localmente?
Retransmisión de vídeo
Alguien estaba usando Twitch para retransmitir en directo su codificación de Bitcoin Core. Esto fue inspirado por Lisa Neigut que lo hace en c-lightning. Creo que es una forma interesante de ayudar a educar a la gente y socializar el proceso de desarrollo. Lo hice como un experimento sólo una vez hasta ahora. Podría ser una práctica útil. Me beneficiaría ver a algunos de ustedes hacer su trabajo. Podríamos hacer un webshow así. Podríamos hacer livestreams obligatorios para poder comprobar lo que la gente hizo durante la revisión. ¿Y si tuviéramos a los 3 mejores revisores de código haciendo eso, el resto de vosotros no estaría interesado en observar eso durante unas horas? Si tienes un RP, podría ser útil para el autor del RP revisar el código y utilizar un flujo de vídeo sólo como una forma de presentar el RP y hablar de él. No deberíamos animarles a abandonar el texto, pero algunos usuarios probablemente trabajen mejor con vídeo y voz. También hay algunos podcasts de revisión de código. A algunas personas les puede gustar más el vídeo que la revisión de texto. Hice un poco de codificación en directo de un cliente lightning en Munich, y a algunas personas les pareció interesante oírte caminar a través de tu proceso mientras escribes software.
Creo que sería útil que los desarrolladores de Bitcoin Core hicieran vídeos así. Creo que hay mucho que aprender de vosotros. Así que consideren hacerlo, ya sea para revisiones o para escribir código. Aunque no hagamos de esto una expectativa. Pero también, programar es generalmente lento, no sólo escribirlo rápido. Googlear «cómo hacer un bucle for en python», etc. Bueno, la gente ve partidos de cricket, no subestimes lo que la gente encuentra interesante.
Club de revisión de Bitcoin Core
Tenemos un club semanal de revisión de Bitcoin Core. Estamos intentando conseguir más revisores. Todo el mundo es bienvenido. Es en IRC. Hasta ahora funciona. En realidad es en 3 horas. Si quieres hacer una en video, entonces tal vez.
Volver a las revisiones
A la hora de coordinar las revisiones, hay limitaciones en cuanto a las etiquetas: ¿con qué frecuencia se van a buscar las etiquetas? Como coordinador, resulta muy útil conocer a los distintos desarrolladores. Tal vez empezar a formar diferentes grupos de interés, no exactamente mantenedores de subsistemas, pero tal vez la gente puede ser voluntaria para diferentes clubes.
El tablero de github es inútil. Pero tal vez podría decir «hey recientemente trabajaste en este código, y este otro PR lo modifica o algo así». O culpar a la automatización. Es difícil automatizar la relevancia en términos de quién es realmente capaz de revisarlo, o quién está interesado en eso. La lista de etiquetas de github no es algo que todo el mundo utilice todo el tiempo. Una notificación por correo electrónico no es lo más adecuado. Tal vez una página web, pero tiene que ser personalizada para cada usuario. ¿Tal vez una aplicación web de solicitud de revisión? O el coordinador puede hacerlo manualmente, y luego automatizarlo eventualmente.
Un coordinador puede decidir no coordinar algo; así que, en cierto modo, el coordinador se convierte en un decisor.
Para determinados RP, como el código consensuado o el código crítico p2p, ¿podrían algunos RP beneficiarse de una revisión sincronizada en grupo, ya sea por chat, vídeo o cara a cara, como en una reunión como ésta? ¿Mejoraría esto la calidad de la búsqueda de errores y el intercambio de buenas prácticas? Esto sería útil para los ACK de concepto. ¿Hay alguien en alguna oficina que tenga un horario coordinado para las revisiones? ¿Como en Chaincode? No. Podría haber una sala de chat de grupo abierta y en funcionamiento. Hay un canal #bitcoin-builds, porque algo de esto requiere discusión. Tener un canal separado para PRs y revisiones.
¿Qué PRs necesitan ACKs de concepto y cuáles no? Sería mejor comunicar esto. Probablemente no todo el mundo es capaz de tomar esa determinación.
Podríamos pedir que los nits se envíen en privado a los autores del PR. Esto despejaría la interfaz de github.
Regularmente se abre un tema sobre el «estado del repositorio». ¿Puedes auto-conceptualizar tu propio PR?
Solíamos tener una forma de listar pull requests triviales, en lugar de los grandes cambios consensuados. Ahora mismo nuestra política es rechazar cualquier PR que actualice las cabeceras de copyright o algunas otras cosas. Nos hemos estado ahogando en- hay un montón de fchanges como linters y cosas que van en eso. ¿Alguien utiliza esas herramientas a nivel local? ¿Son útiles? Muchas de ellas no funcionan en Mac. La interfaz de usuario de estas herramientas no es muy intuitiva. Quiero algo como «comprobar todo desde master». Ahora mismo tengo que darle un rango o algún conjuro que desconozco. Tengo un script sencillo que uso. Debería ser algo como “script/make_linting_easy.sh” o algo así.
Ahora mismo hay abierto un PR de refactorización masiva de cabeceras. Hubo un problema con este. Hay mucho tiempo dedicado a arreglar y mantener scripts para hacer cosas. Los linters son útiles porque apuntan a cosas que normalmente no miramos. ¿Así que debería haber un botón fácil localmente que arregle esto localmente, y luego enviar un PR? Siempre mantengo una pestaña travis-ci abierta porque sé que va a explotar. Pero tal vez un pre-commit git hook, y algún tutorial sobre cómo hacerlo con dos sencillos pasos o algo así, que podría ayudar. Solía ser que travis-ci era más rápido que mi ordenador local pero ahora mismo creo que es al revés.
Podríamos hacer girar VMs de desarrollo para revisar PRs, automáticamente. Así sería más fácil construir y ejecutar pruebas. Los usuarios podrían iniciar sesión con su github login oauth o algo así. Para las pruebas de cartera, tal vez sólo ejecutar pruebas de cartera solamente. Eso estaría bien. A veces no lo hace. ¿Estás compilando con -disable-wallet?
Estoy deseando ver cómo se desarrollan estas ideas. John mencionó recientemente que los conflictos son un problema con respecto a rebasing. Así que empecé a abrir mis PRs en borrador, luego esperar a ver los conflictos, y luego decidir si quiero continuar con ese pull request.