Conversation
… response (qps does not have this property)
added missing setRData
|
any reason this wouldn't be accepted? I'd love to rebase our code. |
tfountain
left a comment
tfountain
left a comment
There was a problem hiding this comment.
Hi Bill,
Apologies for the delay, this one slipped under the radar. I've added some feedback, if you wouldn't mind making some tweaks. A couple of the bug fixes in here have subsequently been fixed on master, so it might be worth merging in the upstream changes as well.
Thanks,
Tim.
|
|
||
| use Dyn\TrafficManagement\Api\Client as ApiClient; | ||
| use Dyn\TrafficManagement\Zone; | ||
| use Symfony\Component\Config\Definition\Exception\Exception; |
There was a problem hiding this comment.
Could you remove this import, as it doesn't appear to be used.
| * @param $end int | ||
| * @return bool|Zone | ||
| */ | ||
| public function getQpsJobs($start,$end) |
There was a problem hiding this comment.
Naming this function createQpsReport would be more consistent with the others, since the API call is a POST, even though what it's doing is returning data.
Also, could you make the parameters camel cased versions of the actual API call args? So $startTs, $endTs.
I would be inclined to make this call return the actual data on success (rather than a response object), just so users of the function don't need to understand the structure of the response object as well:
if ($result && $result->isComplete()) {
return $result->data;
}
| $result = $this->apiClient->post($path, ['start_ts'=>$start,'end_ts'=>$end,'zones'=>[$this->name]]); | ||
|
|
||
| return $result; | ||
| } |
There was a problem hiding this comment.
Same feedback here as for the getQpsJobs() function.
3 participants
and a check for the msgs parameter in response (qps does not have this property)