Evangelos Asimakopoulos code GWI#43
Conversation
alexandros-georgantas
left a comment
alexandros-georgantas
left a comment
There was a problem hiding this comment.
Hello Evangelos 👋 ,
Thank you for taking the time working on this assignment!
I've left some comments as part of my review of this PR and I would love to hear your thoughts!
Thanks,
Alex
| # Podman uses the same Dockerfile syntax. This Containerfile mirrors the Dockerfile. | ||
|
|
||
| # ----- Build stage ----- | ||
| FROM golang:1.22-alpine AS builder |
There was a problem hiding this comment.
Could you please explain your rationale behind keeping this old version of golang?
|
|
||
| Two store implementations: | ||
| - In-memory (default): fast, volatile. | ||
| - File-backed (JSON): persists to a single file. Configure via env vars: |
There was a problem hiding this comment.
I understand your choice of using an in-memory store for the scope of this assignment, however I would like to hear your thoughts about the file backed option.
For your file-based storage why you haven't chosen sqlite for example which would have given you some structure to work with?
There was a problem hiding this comment.
Could you please review this file and suggest if it could be simplified?
| } | ||
|
|
||
| func (chart *ChartAsset) MarshalJSON() ([]byte, error) { | ||
| type alias ChartAsset |
There was a problem hiding this comment.
Could you please explain the need of type alias and what you are trying to solve?
There was a problem hiding this comment.
Could you please suggest potential refactoring of this file as it scores low when it comes to structure and readability?
| writeError(w, http.StatusNotFound, "not found") | ||
| } | ||
|
|
||
| func (s *Server) handleUserFavourites(w http.ResponseWriter, r *http.Request, userID string) { |
There was a problem hiding this comment.
What is you opinion on having a single handler handling all the HTTP methods?
| store = gwi.NewInMemoryStore() | ||
| log.Printf("Using in-memory store") | ||
| } | ||
| server := gwi.NewServer(store) |
There was a problem hiding this comment.
Could you suggest a more graceful approach of initializing the HTTP server?
| ID string `json:"id"` | ||
| Type AssetType `json:"type"` | ||
| Description string `json:"description"` | ||
| Payload json.RawMessage `json:"payload"` |
There was a problem hiding this comment.
Is there a way to define acceptable payload types at compile time instead of just accepting raw data?
| req AddAssetRequest | ||
| wantType AssetType | ||
| wantErr bool | ||
| errorMsg string |
There was a problem hiding this comment.
In your test, are you interested in verifying whether the implementation correctly returns an error when it should, or whether the specific error message returned is the expected one?
Why do you think, in this particular case, the error string itself matters?
| 9) Test coverage | ||
| - Add unit tests that assert: (a) status mapping without relying on string matching (once typed errors exist), (b) duplicate IDs within a batch, (c) duplicate against existing store, (d) large offset/limit interaction after bulk insert, (e) body Content-Type handling, (f) maximum batch size enforcement. | ||
|
|
||
| These improvements would make the bulk endpoint more robust, maintainable, and user-friendly without changing the core behavior clients rely on today. No newline at end of file |
There was a problem hiding this comment.
Could you please explain what is this file for?
3 participants
The code was written mostly by Junie Pro and my role was to supervise and make all the necessary changes in order to provide a deliverable of high standard. The time gained gave me the opportunity to add more tests and functionality.