Return information about job #19
Conversation
Summary of ChangesHello @az-smartling, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the batch processing API by introducing the capability to retrieve detailed information about individual translation jobs. It provides a new method to query job specifics using a project ID and a translation job UID, ensuring that clients can access comprehensive job data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new GetJob method to fetch job information. The implementation is straightforward, but there are a few areas for improvement. I've pointed out the missing context.Context which is a standard practice in Go for I/O operations, and suggested improvements to error handling to make debugging easier. I've also noted a minor issue with how a custom error type is populated. The changes to batch_ent.go look good.
| type Batch interface { | ||
| Create(ctx context.Context, projectID string, payload CreateBatchPayload) (CreateBatchResponse, error) | ||
| CreateJob(ctx context.Context, projectID string, payload CreateJobPayload) (CreateJobResponse, error) | ||
| GetJob(projectID string, translationJobUID string) (GetJobResponse, error) |
There was a problem hiding this comment.
For consistency with other methods in this interface and to follow Go best practices for I/O operations, the GetJob method should accept a context.Context as its first argument. This allows for request cancellation, deadlines, and passing request-scoped values.
| GetJob(projectID string, translationJobUID string) (GetJobResponse, error) | |
| GetJob(ctx context.Context, projectID string, translationJobUID string) (GetJobResponse, error) |
| } | ||
|
|
||
| // GetJob gets a job related info | ||
| func (h httpBatch) GetJob(projectID string, translationJobUID string) (GetJobResponse, error) { |
There was a problem hiding this comment.
The implementation of GetJob should be updated to accept a context.Context to match the change in the Batch interface. While the context might not be used immediately by h.client.Get, adding it makes the function signature consistent with other API methods and is a good practice for functions that perform network requests.
| func (h httpBatch) GetJob(projectID string, translationJobUID string) (GetJobResponse, error) { | |
| func (h httpBatch) GetJob(ctx context.Context, projectID string, translationJobUID string) (GetJobResponse, error) { |
| if code != 200 { | ||
| return GetJobResponse{}, fmt.Errorf("unexpected response code: %d with %s", code, rawMessage) | ||
| } |
There was a problem hiding this comment.
The error message for an unexpected status code is not very helpful because rawMessage is an io.ReadCloser and printing it with %s will not show the response body. The response body for non-200 responses often contains valuable error details from the API. You should read the body and include it in the error message. Also, it's better to use the http.StatusOK constant instead of the magic number 200 for readability and maintainability.
| if code != 200 { | |
| return GetJobResponse{}, fmt.Errorf("unexpected response code: %d with %s", code, rawMessage) | |
| } | |
| if code != http.StatusOK { | |
| body, _ := io.ReadAll(rawMessage) | |
| return GetJobResponse{}, fmt.Errorf("unexpected response code: %d with %s", code, body) | |
| } |
api/batches/batch.go
Outdated
| return GetJobResponse{}, smerror.APIError{ | ||
| Cause: err, | ||
| URL: url, | ||
| Payload: []byte(fmt.Sprintf("%v", rawMessage)), | ||
| } |
There was a problem hiding this comment.
When io.ReadAll(rawMessage) fails, you are creating an smerror.APIError. However, the Payload field is being populated with []byte(fmt.Sprintf("%v", rawMessage)). At this point, rawMessage has already been read (or an attempt was made), so it's likely empty or closed. This will not provide useful information. The Payload field in APIError is meant for the request payload. For a GET request, there is no payload. You should set Payload to nil.
| return GetJobResponse{}, smerror.APIError{ | |
| Cause: err, | |
| URL: url, | |
| Payload: []byte(fmt.Sprintf("%v", rawMessage)), | |
| } | |
| return GetJobResponse{}, smerror.APIError{ | |
| Cause: err, | |
| URL: url, | |
| Payload: nil, | |
| } |
Return information about job