ZeroDragon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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