Api methods now consistently return as associative arrays instead of Result (BC warning) #137
Conversation
|
Please rebase due merge conflicts. |
…eturn as associative arrays instead of Result
| @@ -21,7 +21,10 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
| - Minimal supported PHP version changed from 5.2 to 5.3 by [@chobie]. | |||
| - The `Api::getPriorties` renamed into `Api::getPriorities` by [@josevh]. | |||
| - Remove trailing slash from endpoint url by [@Procta]. | |||
| - Added local cache to getResolutions [@jpastoor]. | |||
| - Added local cache to getResolutions by [@jpastoor]. | |||
There was a problem hiding this comment.
Some rebase issue, because this line was part of another PR.
jpastoor
force-pushed
the
improvement/73-InconsistencyApiCallsReturnType
branch
from
a199525
to
a33afca
Compare
| @@ -173,14 +173,14 @@ protected function clearLocalCaches() | |||
| /** | |||
| * Get fields definitions. | |||
| * | |||
| * @return array | |||
| * @return array|false | |||
There was a problem hiding this comment.
- Is there a test case to confirm, that
falseis returned at some point? - Maybe we should throw exception in
ClientInterfaceon broken/error responses from JIRA instead?
This applies to other similar places where you've added |false to DocBlock.
There was a problem hiding this comment.
Valid points, lets investigate that as part of #135.
Here I just made consistent for what we already have, and was not meant to complete unit test coverage for the methods involved, guess that is work for another time :)
There was a problem hiding this comment.
On second thought its a bit dodgy to make a BC change without adding tests as well. I might add some tomorrow that validate against JIRA responses just like I did with getStatuses etc.
There was a problem hiding this comment.
Luckily changed methods aren't 100% of public class interface. Please create some basic tests for them (at least confirm, that they return non-empty array all the time).
There was a problem hiding this comment.
It's better to wait some time and merge PR with a tests, then merge PR quickly, but without any tests.
| - Added local cache to `Api::getResolutions` by [@jpastoor]. | ||
| - Renamed `Api::api` parameter $return_as_json to $return_as_array by [@jpastoor]. | ||
| - Changed default `Api::api` parameter $return_as_array to TRUE by [@jpastoor]. | ||
| - Api methods (except those that return issue lists) now consistently return as associative arrays instead of Result by [@jpastoor]. |
There was a problem hiding this comment.
Please replace instead of Result with instead of Result class instance.
|
Still WIP, a few more tests are in the pipeline. |
|
Thats it for me for now, rest of the tests can be done in a later PR, maybe even by someone else. |
| @@ -564,7 +559,7 @@ public function search($jql, $start_at = 0, $max_results = 20, $fields = '*navig | |||
| 'startAt' => $start_at, | |||
| 'maxResults' => $max_results, | |||
| 'fields' => $fields, | |||
| ) | |||
| ), false | |||
There was a problem hiding this comment.
CS
each multi-line function call must be on separate line
|
Today’s review completed. |
|
@jpastoor , any updates? |
|
Hard for me to spent time on atm, will have more time in october to finish this and some other stuff :) |
|
I recently saw, that repo collaborators (e.g. me) can add new commits to PR branch (e.g. branch in your repo fork) if PR author allows that, see:
Since I'm the one, who requested changes I already know what needs to be done and you only need to grant me the permission to do it. |
|
Sweet, done |
As discussed in #73 we feel that the Result class does not add anything useful for Api calls that are not lists of issues on which you can paginate/walk.
This PR proposes a change to more consistently return the data as associative array structures instead of Results. This is a BC break for many users, although we expect it to reduce the boilerplate in the client code. (One less unwrapping to do)
Fixes #73