-
Notifications
You must be signed in to change notification settings - Fork 1
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
Remove next groups api #144
Conversation
client/src/services/GroupService.ts
Outdated
@@ -7,13 +7,39 @@ import { Logger } from '@/services/Logger'; | |||
import { ApiCommunicator } from '@/services/ApiCommunicator'; | |||
import { Member } from '@/types/Member'; | |||
|
|||
export type Options = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mr-T-Smyth Para mantener compatibilidad inclui este parametro dado que en diferentes partes del codigo aveces pasamos disintos valores de estos parametros para el mismo endpoint.
Lo hice asi para no hacer tanto cambio, y fue solo necesario en este servicio por ahora, no en el de subject, lo bueno seria evitar usar esos parametros en el futuro pero esto asi evita tener que hacer mas cambios de los necesarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buen laburo! Un par de cosas:
- Por lo que veo hay dos metodos el de getById y getAll que no usan lo parametros opcionales, por lo que no veo que se necesario.
- El resto de metodos todos tienen asJSON en false, asique creo que prefiero que hagas al final como hablaste de que siempre devuelva la response y que esta sea parseada en el Service correspondiente
- El tema de handleNotOK no me termina de convencer como quedo cuando lo hice y creo que seria mejor de la siguiente manera: Que siempre los noOK se atajen en el commonFetch y que tire una excepcion nuestra tipo (por dar un nombre) CommonFetchError() y que adentro tenga los errores tirados por backend si hay asi en el componente todos los errores los manejamos en un try catch y no como ahora en un hibrido de que alguna si y otras no.
El punto 3 capaz que mejor no hacerlo en este pr pero capaz que si en un pr particular
@Mr-T-Smyth con esto estoy de acuerdo, yo dejaria que devuelva el json siempre y dsp cada componente muestra los mensajes de error como quiera( como mencionas en el punto 2 ). El unico problema que veo de cambiarlo ahora es que tendriamos que cambiarlo cuidadosamente de no romper todo, no seria mas facil cambiarlo cuando este separado todo en servicios? capas me estoy equivocando y cambiarlo ahora no es tanto trabajo pero no revise bien el codigo |
Si, tenes razón y es como esta siendo encarado al final, en una rama aparte empece el refactor de lo que seria el punto 2 y 3 juntos pero para ser mergeado despues de terminar el resto |
* remove groups next api, move group methods into group service * include optional Options object in every call * rename to ApiOptions and mark parameters in Options as optionals * add options on calls that used * fix bad call
What && Why
options
parameter where we can pass individual options to calls such asasJSON
andhandleNotOk
, we should stop using this on future calls.Disclaimers / Extra Comments
Please test it!!
Links
How to Review