Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IV-23-23] Objetivo 4 #29

Merged
merged 41 commits into from
Jan 5, 2024
Merged

[IV-23-23] Objetivo 4 #29

merged 41 commits into from
Jan 5, 2024

Conversation

DarckMonster
Copy link
Owner

@DarckMonster DarckMonster commented Nov 30, 2023

  • ¿Hay un camino claro que permita ir desde el código hasta la historia de
    usuario que desarrolla, pasando por el mensaje de commit y el issue
    correspondiente?
  • ¿Se ha intentado cubrir con tests una parte de la lógica de negocio que se
    esté desarrollando?
  • ¿Hay un producto mínimamente viable que describa lo que se va a entregar en
    este objetivo?
  • ¿En este producto mínimamente viable se han priorizado unas clases,
    módulos o paquetes más fundamentales frente a otros, movidos a otro
    PMV/Hito?
  • ¿Estoy leyendo todo lo que marco o simplemente marco lo que se me pone por
    delante?
  • ¿Se han documentado las elecciones de biblioteca de aserciones y del
    de test runner?
  • ¿Te has asegurado que lo que mencionas como biblioteca de aserciones y
    test runners lo son realmente y tienen la misma funcionalidad? ¿O estás poniendo
    unittest como test runner y pytest como biblioteca de aserciones?
  • ¿Esa documentación incluye criterios de aceptación y también criterios de
    búsqueda para las diferentes opciones?
  • ¿Se han seguido los principios F.I.R.S.T. en la creación del test y
    se ha documentado cómo se ha hecho?
  • ¿Se describe claramente en el PMV los tests que hay que pasar para que se
    considere viable?
  • ¿Se han testeado también las excepciones?

@JJ JJ mentioned this pull request Dec 7, 2023
3 tasks
Copy link

@adelahera adelahera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sobre los issues, al menos para mí (y creo que en general es cómo se debe hacer) me es mucho más fácil crear issues específicos para cada problema que vaya surgiendo, en vez de explicar todo lo que vas a hacer en un issue solo. Así es mucho más fácil de entender los commits también, pues sabes que van directos a resolver ese problema en concreto. De la otra forma no se sabe muy bien qué parte del issue va a resolver cada commit.

Además de que hay unos cuantos commits que no siguen las mejores prácticas. Recuerda que cada commit debe ir dirigido a resolver un issue, y que no está de más si incluyes una descripción en el cuerpo del commit explicando qué has solventado.

src/_test.ts Outdated Show resolved Hide resolved
docs/tests.md Outdated Show resolved Hide resolved
@DarckMonster
Copy link
Owner Author

**adelahera ** left a comment

Sí, tienes razón, me di cuenta en cuanto iba haciendo commits. Planee que los commits iban a ir sobre las tres cosas...

@DarckMonster DarckMonster requested review from adelahera and JJ December 13, 2023 17:18
Copy link

@adelahera adelahera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Veo que has implementado lo de devolver la más barata, así que yo ahora lo veo bien 👍

Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No lo he revisado completo, pero lo de usar caminos absolutos bloquea la superación del objetivo. Por favor, dale una vuelta (y el resto de los detalles, por favor) Y muchas gracias a @adelahera por la revisión.

docs/tests.md Outdated Show resolved Hide resolved
iv.yaml Outdated Show resolved Hide resolved
src/_test.ts Outdated Show resolved Hide resolved
src/componente.ts Outdated Show resolved Hide resolved
Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creo que has hecho un esfuerzo bastante completo en la documentación y en el código, pero el problema es que este último resulta algo complejo de entender para quien lo revisa, y por lo tanto complicado saber si satisface o no la historia de usuario. Por favor, dale una pequeña vuelta empezando por los issues y reescribe las partes de código que creas que están afectadas.

iv.yaml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/componente.ts Outdated Show resolved Hide resolved
src/scraping.ts Outdated Show resolved Hide resolved
src/scraping.ts Outdated Show resolved Hide resolved
src/scraping.ts Outdated Show resolved Hide resolved
docs/tests.md Show resolved Hide resolved
docs/test_baratas.txt Outdated Show resolved Hide resolved
@DarckMonster DarckMonster requested a review from JJ December 19, 2023 19:44
Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Los comentarios siguen así. Si no están cerrados, hay que revisarlos uno por uno a ver si se ha hecho algo al respecto. Por favor, cambia o borra el texto correspondiente o indica el commit en el que se han cerrado, resolviéndolos.

docs/tests.md Show resolved Hide resolved
docs/tests.md Outdated Show resolved Hide resolved
@JJ
Copy link

JJ commented Dec 20, 2023

... y cuando termines pide revisión

@DarckMonster DarckMonster requested a review from JJ December 20, 2023 12:34
Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Te rogaría que revisaras, una vez más, todos los comentarios, pero sobre todo que no resuelvas comentarios que no están, en realidad, resueltos. En la mayoría de los casos, cuando se hace un comentario específico sobre un código o texto específico, es para que ese texto se altere o directamente se suprima, si lo que se cuestiona es su necesidad. Mantener el texto y cerrar el comentario sólo lleva a más trabajo por las dos partes, pero especialmente la mía. ¿Crees que es la mejor forma de invertir nuestro tiempo, pero sobre todo el tuyo?
Por otro lado, convendría que miraras los errores frecuentes. Mira especialmente esto:

Usar "flags" para decidir si una cosa se ha encontrado o no. Los bucles se tienen que tratar de evitar siempre que haya alternativas, y la forma correcta de encontrar algo es filtrar un array que devuelva los que cumplen el criterior, o uno vacío si no hay ninguno. Trabajar con índices y bucles es herencia de tiempos pasados en los que los lenguajes no tenían órdenes de más alto nivel para trabajar con ellos.

También:


Hay que tratar de consultar cuál es la buena práctica actual para las estructuras de un lenguaje, en todos los sentidos. Por ejemplo:

Cómo declarar las variables de instancia en TypeScript
Qué tipo de convención usar cuando se trata de variables privadas de clase.
Cómo declarar los getters y setters en JS/TS.

(en tu caso, qué son las clases sin funciones)

En general, dale también una vuelta al resto de los errores frecuentes, y trata de evitarlos en la próxima revisión. Ánimo, que ya queda menos.

docs/tests.md Outdated Show resolved Hide resolved
src/componente.ts Outdated Show resolved Hide resolved
src/scraping.ts Outdated Show resolved Hide resolved
src/scraping.ts Outdated Show resolved Hide resolved
@DarckMonster DarckMonster requested a review from JJ December 26, 2023 12:36
Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, te has centrado en el scraping, que se queda un poco a medio camino de la lógica de negocio en la HU, pero es válido para este objetivo. También ten en cuenta los comentarios antes del siguiente PR.

src/tienda.ts Show resolved Hide resolved
src/tipo_componente.ts Outdated Show resolved Hide resolved
@DarckMonster DarckMonster merged commit 8ec7852 into main Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants