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

[WIP] 7 - Implementar HTTP DELETE/plants/{id} #21

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

javierlopezdeancos
Copy link
Member

#7

Esta PR es un DRAFT para revisión

Closes #7

@javierlopezdeancos javierlopezdeancos changed the title 7 - Implementar HTTP DELETE/plants/{id} [WIP] 7 - Implementar HTTP DELETE/plants/{id} Oct 29, 2019
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

De momento tiene buena pinta, quiero bajarlo y probar. No obstante te dejo algunas cosillas que he visto

plants/model.go Outdated
}

// SearchQueryByIndexNameResult wraps a search result
type SearchQueryByIndexNameResult map[string]interface{}
Copy link
Member

Choose a reason for hiding this comment

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

El index es algo que se pone en el cliente, yo lo llamaría SearchQueryResult

plants/model.go Outdated
}

// SearchQueryByIndexName struct
type SearchQueryByIndexName struct {
Copy link
Member

Choose a reason for hiding this comment

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

El index es algo que se pone en el cliente, yo lo llamaría SearchQuery

Copy link
Member Author

Choose a reason for hiding this comment

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

eliminarias ese IndexName string del struct entonces?

Copy link
Member

Choose a reason for hiding this comment

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

sí, así es reutilizable

plants/delete.go Outdated
@@ -0,0 +1,9 @@
package plants
Copy link
Member

Choose a reason for hiding this comment

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

Yo dejaría ésto como un método en el main.go, para no tener un paquete con sólo un método público, o al menos en un fichero dentro del paquete raíz

Copy link
Member Author

Choose a reason for hiding this comment

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

Bueno, yo también he tenido esa duda, al final, desde mi opinión inexperta, lo hice así para tener un dominio que no dependa de ninguna implementación, en este caso, nuestro dominio son las planticas y su gestión, en el main tenemos el problema que tenemos un buen salpicado de marisco de implementación del servidor, con gin, elastic logs tracking and monitoring

Copy link
Member Author

Choose a reason for hiding this comment

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

la idea era tener un paquete plants de nuestra entidad de dominio y aquí empiezan mis dudas

Copy link
Member Author

Choose a reason for hiding this comment

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

según veo y a efectos practicos, un cliente lo unico que debería pedir es un search('plants'),
ese plants se lo daría el contexto de Gin

Copy link
Member

Choose a reason for hiding this comment

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

crearía un api.go en el raíz, y allí todos estos metodicos

plants/search.go Outdated
@@ -0,0 +1,11 @@
package plants
Copy link
Member

Choose a reason for hiding this comment

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

Lo mismo que para el Delete, lo dejaría en el main.go, o al menos en un fichero dentro del paquete raíz

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.

Implementar HTTP DELETE/plants/{id}
2 participants