(#9) Add PR template #10
Conversation
mdelapenya
requested review from
antoniocrevel,
avcconti,
felixortegam,
javierlopezdeancos and
manudevelopia
mdelapenya
added
documentation
mdelapenya
force-pushed
the
9-pr-template
branch
from
6398cb8 to
d7984b1
Compare
|
|
||
| - [ ] Mi código sigue las normas de estilo del proyecto | ||
| - [ ] He realizado una auto-revisión de mi código | ||
| - [ ] He comentado mi código, en particular las áreas difíciles de entender |
There was a problem hiding this comment.
No estoy muy agree con el tema de los comentarios en el código.
Yo soy de los que pienso que un codigo bien escrito y semantico no hace falta comentarlo y que los comentarios son documentación a mantener que suele quedar muerta.
There was a problem hiding this comment.
Coincido con @javierlopezdeancos. La recomendación es sólo documentar si es explícitamente necesario y sólo para explicar el porqué de ese código, nunca el qué, que lo debe decir el propio código.
There was a problem hiding this comment.
Que exista el check NO significa que haya que poner comentarios. Puedo rehacer la frase para poner:
- He comentado mi código, en particular las áreas difíciles de entender
+ He comentado las áreas difíciles de entender de mi códigoThere was a problem hiding this comment.
De todas maneras, es lo que dije, tener una plantilla a la que enfrentarse y se origine un proceso mental previo a la revisión, de manera que no se envíe cualquier cosa, sino que sea algo bien pensado
There was a problem hiding this comment.
Yo es que pensaba en el equilibrio entre lo que dice @mdelapenya y eliminar burocracia.
- He comentado mi código, en particular las áreas difíciles de entender
+ He comentado las áreas difíciles de entender de mi códigoPara mi es hace pensar si el dev tiene que hacerlo, cuando es algo que, como comenta @antoniocrevel el 95% de los casos va a ser uncheck.
There was a problem hiding this comment.
Ojo que me parece bien quitar este item :)
There was a problem hiding this comment.
He quitado el item en un nuevo commit. Aunque viendo esta PR, donde es necesario poner comentarios indicativos sobre qué hacer en cada elemento del template, me entran dudas si volverlo a poner...
There was a problem hiding this comment.
No creo que haya que eliminarlo, porque cubre un aspecto que es importante. Quizá sólo sea revisar de nuevo la redacción.
¿Qué debe revisar el que quiera crear la PR? Que su código expresa por sí sólo lo que quiere hacer, y que aquellos fragmentos del mismo que lo necesitan, están comentados.
No sé si lo de abajo lo mejora, creo que, paradójicamente, me ha quedado algo complejo:
- He comentado las áreas difíciles de entender de mi código
+ Mi código es legible e inteligible y los fragmentos complejos están comentados para explicitar porqué lo he codificado de esa maneraIgual la frase que puse arriba, algo retocada, es más sencilla y enfatiza que sólo hay que comentar ciertas cosas:
- He comentado las áreas difíciles de entender de mi código
+ Mi código expresa por sí sólo lo que quiere hacer, y sólo están comentados que aquellos fragmentos que lo necesitan| ## Checklist: | ||
| <!-- Marca aquellos elementos satisfechos por esta PR, y deja sin marcar aquéllos no satisfechos --> | ||
|
|
||
| - [ ] Mi código sigue las normas de estilo del proyecto |
There was a problem hiding this comment.
En go no se formatea el codigo automaticamente? en el caso de no u otros lenguajes, soy de actualizar la tarea, dejar esta responsabilidad al desarrollador no me gusta sobre todo cuando es una tarea de 10 min que se tarda en automatizar, al menos en js con prettier.
La democracia a fracasado, mirad Grecia o las 4 elecciones en 4 años de nuestro país jajaja 🎩
There was a problem hiding this comment.
Normas de estilo no tiene por qué ser el tabulado, la posición del paréntesis, los espacios. Para mí, además de eso, es el nombrado de variables, el uso de ciertos patrones, etc
There was a problem hiding this comment.
Y sí, Go se formatea sólo, pero voy más por el segundo nivel, el del estilo de programación
There was a problem hiding this comment.
De acuerdo contigo, tenemos las normas de estilo del proyecto, quiero decir, este check será check o uncheck con respecto a unos criterios no?. Lo digo por pensar en devs externos o que pasan de manera tangencial en el proyecto, de como obtener todo el contexto.
There was a problem hiding this comment.
Exacto, en algún momento tendríamos un ficherito con las normas de estilo. Pero vamos, que estamos de momento bastante lejos de ello (en este proyecto)
| - [ ] He comentado mi código, en particular las áreas difíciles de entender | ||
| - [ ] He actualizado la documentación | ||
| - [ ] Mis cambios no generan nuevos warnings | ||
| - [ ] He añadido tests que prueban, o bien que mi fix es efectivo, o que la funcionalidad funciona como se espera |
There was a problem hiding this comment.
Hay algún caso en el que los cambios puedan generar nuevos warnings y pueda ser mergeada?
There was a problem hiding this comment.
Creo que el éxito está en conseguir que la persona que envía una PR se responsabilice de no enviar una PR como Billy el niño, así de botonazo caliente. Al tener que rellenar este template debería hacerle pensar sobre lo que está enviando
| - [ ] He comentado mi código, en particular las áreas difíciles de entender | ||
| - [ ] He actualizado la documentación | ||
| - [ ] Mis cambios no generan nuevos warnings | ||
| - [ ] He añadido tests que prueban, o bien que mi fix es efectivo, o que la funcionalidad funciona como se espera |
There was a problem hiding this comment.
Si usaramos el caso de arriba semantic commits, no haría falta este check (se podría ver con la etiqueta test en los commits). Aunque no me opongo si lo véis mas claro así.
There was a problem hiding this comment.
Es lo mismo, responsabilizar a quien envía la PR para que pruebe en local
There was a problem hiding this comment.
con el pre-commit esto estaría cubierto, otra cosa sería algo así como, he hecho otras comprobaciones a parte de lanzar los test locales?
También añadiría algo como
creo que la descripción de esta PR tiene la documentación necesaria, gifs, screen records, screen shots, para entender su contexto?
There was a problem hiding this comment.
Este check es para verificar que se ha probado en local: he arrancado la aplicación, sus dependencias, y pruebo el código, por tanto los tests son fiables. No dejarse llevar por el efecto "tiene buena pinta".
There was a problem hiding this comment.
Creo que es importante añadir lo que ha dicho @javierlopezdeancos para auto revisar que la PR está correctamente documentada.
También añadiría algo como
creo que la descripción de esta PR tiene la documentación necesaria, gifs, screen records, screen shots, para entender su contexto?
mdelapenya
force-pushed
the
9-pr-template
branch
from
d7984b1 to
3d52e46
Compare
mdelapenya
dismissed
felixortegam’s stale review
Los cambios han sido aplicados
mdelapenya
force-pushed
the
9-pr-template
branch
from
ce1096e to
95a5522
Compare
|
He añadido vuestras sugerencias, gracias por el debate!! |
¿Qué hace esta PR?
Esta PR añade un template para las PR
¿Por qué es importante?
Queremos que quien envíe código para ser revisado aporte el máximo de información para que esta revisión sea efectiva y eficiente, por su sencillez y por su facilidad. Por tanto pedimos que se rellene este template aportando la máxima información posible.
¿Cómo se prueba esta PR?
Tipo de Cambio
Checklist:
Issues relacionadas
Closes #9
Checklist del/a Autor/a