-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/pet #4
Feature/pet #4
Conversation
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.
👍
src/app/handler/pet/pet.handler.go
Outdated
// @Success 200 {object} dto.PetDto | ||
// @Failure 500 {object} dto.ResponseInternalErr "Internal service error" | ||
// @Failure 503 {object} dto.ResponseServiceDownErr "Service is down" | ||
// @Router /v1/pet/ [get] |
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.
To follow convention this should be /v1/pets
. Note the s
and trailing slash.
src/app/handler/pet/pet.handler.go
Outdated
// @Failure 400 {object} dto.ResponseBadRequestErr "Invalid request body" | ||
// @Failure 500 {object} dto.ResponseInternalErr "Internal service error" | ||
// @Failure 503 {object} dto.ResponseServiceDownErr "Service is down" | ||
// @Router /v1/pet/{id} [get] |
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.
Also here
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.
it was grouped by pet := GroupWithAuthMiddleware(r, "/pet", authGuard.Use)
should i change this
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.
yep
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.
for 2,000 lines change, this is impressively good pr.
} | ||
|
||
func (s *Service) FindOne(id string) (result *proto.Pet, err *dto.ResponseErr) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
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.
Please change all context to use the HTTP request context. I'm sure that fiber provide it.
Incoming requests to a server should create a Context, and outgoing calls to servers should accept a Context. The chain of function calls between them must propagate the Context, optionally replacing it with a derived Context created using WithCancel, WithDeadline, WithTimeout, or WithValue. When a Context is canceled, all Contexts derived from it are also canceled.
Sorry for all the test for have to fix tho D:
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.
💀
src/app/handler/pet/pet.handler.go
Outdated
// @Failure 400 {object} dto.ResponseBadRequestErr "Invalid request body" | ||
// @Failure 500 {object} dto.ResponseInternalErr "Internal service error" | ||
// @Failure 503 {object} dto.ResponseServiceDownErr "Service is down" | ||
// @Router /v1/pet/{id} [get] |
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.
yep
// @Failure 500 {object} dto.ResponseInternalErr "Internal service error" | ||
// @Failure 503 {object} dto.ResponseServiceDownErr "Service is down" | ||
// @Router /v1/pet/ [get] | ||
func (h *Handler) FindAll(c router.IContext) { |
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.
drop the I IContext
, It's historical reason. We want to move on, not repeat itself. Also to reduce techinal dept for the future :)
Shortly, I
prefixed stand for Interface
which many OOP-language like to use in the past. Nowadays, only c# still use this naming convention.
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.
i agree, i just use it because someone created before lamo
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.
Do you mention P Mo *cough* *cough*.. P' Modem?
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.
It's time to move on, if you think something isn't right you can discuss.
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.
I'll change all in next pr🫡 i think this pr is toooo much 🥶
Update pet service & pet handler
Pet service
Add pet services
Add pet services test
Pet handler
c *fiber.Ctx
torouter.IContext
ChangeView(id)
toChangeView(*dto.ChangeViewDto) (bool, *dto.ResponseErr)
Add pet handler
Add per handler test
PetDto
Mock
ALL UNIT TEST PASSED
Problems
./src/main.go