Code review

[ZeroDragon]

Conclusiones

A nivel general me parece bien, se resolvió el problama, las instrucciones del README no funcioan al 100% porque hay un problema en configuración del docker-compose.yml, me gustó que se haya separado la lógica de negocio de la capa web pero se desaprovechó esta separación en los tests.

Hubo un desarrollo incremental y conoce herramientas de desarrollo como formatters, linters, etc. pero no las aplica correctamente, sobretodo en el punto de configuración.

Punto por usar docker pero no hizo ua correcta separación y manda un Dockerfile de desarrollo a producción

En términos generales me parece un candidato con buenos conocimientos pero que falta pulirlos

Uso de GIT y CI

Pareciera que se tomó de una plantilla y no se ajustó apropiadamente, en la parte de instalación de dependencias se busca un archivo que no existe en la raí

Según la historia de git se ve un trabajo incremental, primero hizo un bootstrap del proyecto, luego hizo la solución al problema para luego agregarle un endpoint

Linters y demás herramientas de desarrollo

Se usa black para formateo(según el Makefile) pero esto no se valida en el CI, las reglas de formateo se especifican solo en el Makefile de modo que si alguien no usa eso puede obtener resultados diferentes, se debería usar un archivo de configuración

Las reglas a aplicar por flake8 (linter) se especifican solo en el CI y no en un archivo de configuración, lo cual no permite hacer una validación fuera del entorno de CI

Uso de python

Se separó la lógica de negocio en una clase pero se usó un dataclass, los cuales no "deberían" tener lógica mas allá de la que compete a sus propios atributos ni tampoco mentener estado. Al usar un dataclass se debió usar solo funciones, parece que en este punto primó el uso del framework(fastapi) que recomienda usar dataclass pero para la validación de datos y no para la lógica

Se usa map en el procesamiento de la informacion pero no se justifica su uso, pareciera que se le quiso dar un "toque funcional" en lugar de ir con los "idioms" de python que sería en este caso usar listas por compresión

El nombrado de variables podría mejorar un poco, se están usando nombres que no dan suficiente contexto, por ejemplo d, ju_ind, etc.

Hay mensajes de logging a nivel de debug pero en ninguna parte se especifica el nivel de logging el iniciar la aplicación por lo que no se están mostrando

Se está usando un middleware para soportar cors pero se deja muy abierto, considerando que la aplicación está desplegada debería tener una forma de parametrizar los hosts permitidos

Testing

Se hicieron pruebas pero no se pone mucho contexto en ellas, además se hicieron directamente desde la capa web, la solución al problema está en una clase de python por lo que se podría haber hecho pruebas unitarias y luego de integración con la capa web.

Documentación

Se documentaron algunas funciones pero se puso solo una descripción corta en lugar de una explicación

Si existe documentación del API usando swagger pero esto es algo que genera automáticamente fastapi, punto extra por agregar descripción en la raíz

Ejecución

En el docker-compose se define que se require un archivo backend/.env pero en ninguna parte se especifica qué debe haber en ese archivo, lo cual no permite ejecutar la aplicación

Se tiene un único archivo Dockerfile el cual se usa para desarrollo(usando un flag --reload) y también para el despliegue(mediante Google Cloud Run)

[ZeroDragon]

en resumen: Puntos a pulir, pero buenos resultados.
Duda: Si tuvieras 1 semana (hipotéticamente hablando) y con este feedback, qué es lo que harías para la versión 2.0?

[tyoc213]

Hola @ZeroDragon, es buen feedback 👍 , hay algunas cosas que cambiaría

Docker

• el .env realmente no lo termine usando porque no use BD y no lo agregue al git, localmente esta vació pero si se me olvido subirlo al repo (ya que efectivamente iba agregando al git lo que iba usando y dejando fuera lo que ya no) y no lo probe todo desde cero para notar el archivo faltante por eso me deja construir localmente y deployar sin problemas (lo voy a subir para uso a futuro ya este ahi).
• Para la parte del deploy y desarrollo local lo haría pasando variables al docker y así se seleccionar que hacer de acuerdo a ellas (quitar el reload), actualizaría el Makefile con los correspondientes comandos.
• El logging level seria por ambiente si es productivo los mas bajos de info no irían y en desarrollo y relativos si lo mismo de que URLs pueden hacer petición seria para el CORS.

También podrían ser diferentes docker y hacer los comandos de make correspondientes.

github

• Modificar el github action para usar black tmb en vez de flake8.
• Sería importante en algún momento armar el CD para el proyecto tanto para desarrollo como para producción.

testing

• Agregar pruebas para la lógica de la clase standalone.

usar githooks

Para resolver las incongruencias entre lo que se usa localmente y en el CI para no tener que andar corriendo los comandos de make black en particular.

code style

• Creo que podría ser debatible que meter y que no dentro del dataclass, pero si fuera un proyecto con un code style previo sería cuestión de seguirlo.
• Mmm, sin duda las comprehensiones son mas pythonicas, pero hacerlo mas funcional tampoco afecta... no creo que seria necesario justificar el uso de uno sobre otro en la preferencia del programador... pero seria cambiable si hay un code style que seguir. También lo puse para que sepan que la programación funcional no me es "tan ajena".

python

• Anotado los nombres de vars no tan crípticos aunque los considere intermedios.
• Extendería la descripción en particular de los métodos centrales, aunque prefiero las descripciones que las explicaciones.

extra

• Agregar open telemetry

[tyoc213]

Una nota extra, realmente no he usado fastapi + allá de saber de él y algún contacto superficial, pero creo que era una buena oportunidad de agarrarlo (xq esta cool).