Skip to content
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

Inconsistency in return type of API calls #73

Open
jpastoor opened this issue Feb 28, 2016 · 3 comments · May be fixed by #137
Open

Inconsistency in return type of API calls #73

jpastoor opened this issue Feb 28, 2016 · 3 comments · May be fixed by #137
Milestone

Comments

@jpastoor
Copy link
Collaborator

jpastoor commented Feb 28, 2016

The API::api() method has a parameter called $return_as_json.

Currently I see two problems

  1. When set to true, data is not returned as JSON but as associative array instead.
  2. The API class has many helper methods that wrap around the api() method. About half of them return as associative array, the others as Result.

I haven't looked into this too deeply yet but it might be nice to

  • standardize the wrapper methods to one return type
  • and/or propogate the return_as_json by adding that option in the wrapper method parameter list as well
  • maybe rename return_as_json to return_as_array
@aik099
Copy link
Member

aik099 commented Feb 28, 2016

Having api method on Api class is confusing enough as it is 😄

Looking at Api::api method the $return_as_json in fact means:

  1. get data
  2. parse it into array using json_decode
  3. return it

Without this $return_as_json parameter the same array as above is wrapped with Result class, which is some kind of iterator creation attempt, that walks over queried issues.

Maybe doing a rename on that parameter will solve that problem. We can't really introduce apiJSON method to solve it.

@aik099
Copy link
Member

aik099 commented Jul 23, 2016

After pondering about this for a while I have some usable responses now:

standardize the wrapper methods to one return type

👍

The Result class obviously is tailored for representing issue list that Walker can iterate upon. For all other response types (e.g. project versions, statuses, etc.) the only Result::getResult method would return underlying array as-as.

Since it's 2.0.0 we can:

  • change default value for $return_as_json to true (from false)
  • in place (1 or 2 places most likely), where we're getting issue list back specify $return_as_json as false

and/or propogate the return_as_json by adding that option in the wrapper method parameter list as well

@jpastoor , what do you mean?

maybe rename return_as_json to return_as_array

I've created #119 with exactly that proposition.

@jpastoor
Copy link
Collaborator Author

I'll write a PR for this tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants