Ecaresoft java fix #20
Conversation
benoror
requested review from
luissifu,
safloresg,
Gnikangel,
cvazquezecare and
adeluna
|
Hola @LuisAlvarez98, gracias por tu aportación! En realidad lo mejor sería que el código sea lo mas semántico y auto-documentado posible, y cuando no es posible si es válido poner comments. Por otro lado, ver todos los cambios en un solo commit se complica al hacer review 🙈 crees que se pueda hacer más granular y semántico el PR? (https://github.com/ecaresoft/ecs-tests/blob/master/doc/commits.md) Also, podrías agregar más cambios de fondo y no sólo de forma al código? Igualmente estaría bien si agregaras pruebas unitarias significativas, por ejemplo podrías armar "Mocks" de datos y testear con JUnit. Ojalá lo puedas hacer antes de la siguiente entrevista. Saludos! |
|
Claro, muchas gracias!
Saludos
…________________________________
From: Ben Orozco <notifications@github.com>
Sent: Monday, November 5, 2018 7:10 PM
To: ecaresoft/ecs-tests
Cc: Luis F. Alvarez; Mention
Subject: Re: [ecaresoft/ecs-tests] Ecaresoft java fix (#20)
Hola @LuisAlvarez98<https://github.com/LuisAlvarez98>, gracias por tu aportación!
En realidad lo mejor sería que el código sea lo mas semántico y auto-documentado posible, y cuando no es posible si es válido poner comments. Por otro lado, ver todos los cambios en un solo commit se complica al hacer review 🙈 crees que se pueda hacer más granular y semántico el PR? (https://github.com/ecaresoft/ecs-tests/blob/master/doc/commits.md)
Also, podrías agregar más cambios de fondo y no sólo de forma al código? Igualmente estaría bien si agregaras pruebas unitarias significativas, por ejemplo podrías armar "Mocks" de datos y testear con JUnit. Ojalá lo puedas hacer antes de la siguiente entrevista. Saludos!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#20 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/APRr0LlV7pdHXfO0LwlgSiwa5x0uEyChks5usOGUgaJpZM4YPael>.
|
…ases of validation for the tax object
benoror
requested review from
lorenalama
and removed request for
luissifu,
safloresg and
adeluna
There was a problem hiding this comment.
Gracias @LuisAlvarez98, tendrás chance de investigar sobre JUnit y agregar algunos tests antes de nuestra entrevista?
@lorenalama pls review 😄
|
Hola @LuisAlvarez98, revisé todos los commits que estaban, y te agregué unos comentarios ya en la última versión de la clase. Hay una parte en el código que itera listas para convertirlas en Map, podría interesarte esto: |
Puse los comentarios en el codigo, gracias.