-
Notifications
You must be signed in to change notification settings - Fork 14
Add options to ListJobs func. #26
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
Add options to ListJobs func. #26
Conversation
api_job.go
Outdated
| "count": Optional{0, 0}, | ||
| "to": Optional{"", "?"}, | ||
| "from": Optional{"", "?"}, | ||
| "to": Optional{0., 0.}, |
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.
Currently the API server is returning a float instead of a string.
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.
That's right. The API server does not return a string.
But why don't you use int? Both From and To are offset numbers, so they are always int.
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.
During development, Go's error message showed me that it was a float, and I followed it.
As you say, int is satisfactory. I fixed it on d4c3b14.
hkdnet
left a comment
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.
Thank you for your PR.
This is almost fine but I wrote some comments.
api_job.go
Outdated
|
|
||
| func (options *ListJobsOptions) WithStatus(status string) *ListJobsOptions { | ||
| if status == "running" || status == "queued" || status == "success" || status == "error" { | ||
| options.status = status |
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.
This line contains leading spaces. Please use onnly tabs for indent.
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.
Sorry. I fixed it.
api_job.go
Outdated
| "count": Optional{0, 0}, | ||
| "to": Optional{"", "?"}, | ||
| "from": Optional{"", "?"}, | ||
| "to": Optional{0., 0.}, |
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.
That's right. The API server does not return a string.
But why don't you use int? Both From and To are offset numbers, so they are always int.
|
Thank you for your comments! I added some commits. |
hkdnet
left a comment
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.
LGTM
Thank you
|
Thank you too! |
Hi,
I had Treasure Data support taught us that the parameters
from,toandstatuscan be specified with the end point of Treasure Data API/v3/job/list(*).By merging this pull request, it becomes possible to specify the parameters as shown below.
I made a new function ListJobsWithOptions in consideration of the backward compatibility of ListJobs, but I think that there is also an option that ListJobs takes arguments in strictness.
I would like to hear the thoughts of the contributors.
(*) Documents contains several parameters, including the old ones, but I understand that the currently available parameters are
from,to,statusandslower_than. The last one is written in the document to be avoided, so the implementation added in this pull request is not support this.https://support.treasuredata.com/hc/en-us/articles/360001260527-REST-API#get-v3joblist
https://support.treasuredata.com/hc/en-us/articles/360020228934-Treasure-Data-Job-APIs