healthcare: add search study by tags #1015
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
This is slick. We considered adding this option directly to the client library package, but were hoping for a cleaner solution. This is definitely a good workaround in the meantime (forever?) since it's better than nothing.
@broady and @jadekler, should we add a query param option? We decided against it a while back. I think this is the only API that would use it since the Healthcare API conforms to an external spec -- it's not a normal GCP API. But, it might be nice to avoid having to do this every time you want to call one of the Healthcare endpoints that requires query parameters.
healthcare/dicomweb_study_search.go
Outdated
|
|
||
| // Create a struct to append the query parameter because the library does not currently | ||
| // natively support this. | ||
| type queryParam struct { |
There was a problem hiding this comment.
Name this queryParamOpt or queryParamOption?
There was a problem hiding this comment.
Renamed to queryParamOpt.
healthcare/dicomweb_study_search.go
Outdated
| healthcare "google.golang.org/api/healthcare/v1beta1" | ||
| ) | ||
|
|
||
| // Create a struct to append the query parameter because the library does not currently |
There was a problem hiding this comment.
Please start the comment with the thing being described.
Something like:
// queryParam is an googleapi.Option (https://godoc.org/google.golang.org/api/googleapi#CallOption) which adds query parameters to an API call.
A link to what interface this is fulfilling also seems helpful.
There was a problem hiding this comment.
healthcare/dicomweb_study_search.go
Outdated
| // Refine your search by appending DICOM tags to the | ||
| // request in the form of query parameters. This sample | ||
| // searches for studies containing a patient's name. | ||
| resp, err := call.Do(queryParam{"PatientName", "Sally Zhang"}) |
There was a problem hiding this comment.
It's generally good practice to include the struct field names when initializing a struct. This is probably OK since it's in the same package, but it might be slightly more clear/less error prone to do:
patientName := queryParam{key: "PatientName", value: "Sally Zhang"}
resp, err := call.Do(patientName)(Or inline if it doesn't seem too long.)
healthcare/dicomweb_study_search.go
Outdated
| return fmt.Errorf("Get: %v", err) | ||
| } | ||
|
|
||
| fmt.Fprintf(w, "Found studies: %+v\n", resp) |
There was a problem hiding this comment.
Should we print this more nicely since it's just an http.Response (https://godoc.org/net/http#Response)? Any expectations about the format? Check the status code?
Sorry, pretty out of the loop. Is there a relevant part of this CL I can look at? And, could you link to the historical context you're referencing? |
I looked but can't find a CL. I may have only done it locally. But, this part defines a new Option: https://github.com/GoogleCloudPlatform/golang-samples/pull/1015/files#diff-01edc149e7aa2090350558b0c0bf8970R28. Should we add a QueryParam option to https://godoc.org/google.golang.org/api/googleapi#CallOption? |
healthcare/dicomweb_study_search.go
Outdated
|
|
||
| package snippets | ||
|
|
||
| // [START healthcare_search_studies] |
There was a problem hiding this comment.
Is this the right region tag? I don't see it in the tracker (might be missing it).
There was a problem hiding this comment.
It's actually not in the tracker yet, I'll add it now.
No description provided.