-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Standardize return types for paginated and unpaginated results #8
Comments
@jgaehring I'm glad you're bringing this up! Good things to point out. My understanding is that if the client API is handling pagination internally (accepting a page # in the filters object and deciding to call {
self: 2, // current page number
first: 0, // first page number, is this necessary, though?
last: 4, // last page number
list: [
// log objects
]
} This brings up another question: Should the client library default to returning ALL records by default? I think we should at least have a flag |
Right now it is being handled pretty similarly in farmOS.py - I tried to implement the python client as similarly I could to farmOS.js. The library starts with a public For the most part these are working the same;
# inside _get_records()
if 'page' in filters:
data = self._get_record_data(filters=filters)
if 'list' in data:
return data['list']
else:
return data # return the response for errors?
...
I hope that makes sense? |
Ah I like that. Makes sense. Have you implemented this yet? Or should we agree to do so?
That is a good question, one we've been asking from the start, I believe. I think we settled, at least tentatively, on defaulting to all pages, which is why I implemented it that way, but it's definitely a question that's open for debate. I'm trying to remember some of the arguments for/against from previous discussions with @mstenta. IIRC the general thrust was that we should start by defaulting to all pages, and only reign it in if applications required some sort of default limit. And for now no such requirements exist.
Ok, I think that all makes sense. So just to confirm, as of now, you are just returning the raw array/list as the response value, whether it's a paginated or non-paginated response, correct? Is this how we want to keep it then, or do we want to try to implement the |
Yeah, that is correct. I only return a list of objects for paginated and non-paginated views. I think that both paginated and non-paginated responses should be consistent - either return just the array/list, or return the list and As far as including the I think in an object/dictionary, yeah, it would be easy to implement - just include |
Mm, not really, unfortunately. Only a single return is allowed. It's becoming a little more common in JS to use arrays like a tuple (ie, just an array of 2 values), particularly as a return value (eg, React Hooks), but to do so here would add some definite weirdness to the API that most JS devs would look askance at. It would be much more idiomatic to revert to a structure more like the original response value, an object with 4 properties (self, first, last, list), which means we're pretty much back where we started from. But perhaps this is where the libraries diverge a bit? After all, we're working with each language's native data structures once we get to the return value—that's part of the point of the libraries, after all—and Python dictionaries aren't strictly the same as JS object literals anyways, so perhaps it's no more or less equivalent for farmOS.py to return two values, and JS to return an object with nested properties. Structurally, they're still similar enough. Or perhaps the JS object could be 2 nested objects, instead of 4: {
page: {
self: 0,
first: 0,
last: 2,
},
list: [
// log objects
],
} Maybe that's more analogous, or maybe that's just overcomplicating it. Either way, I think I'm going to make some tentative changes to farmOS.js to make sure it's at least internally consistent for now. I've played around with it a little and it's a simple one liner to add the |
This sounds good! I like your idea for your structure having And referring to different languages and data structures, this does seem like a good place to diverge. I will look into what is the more pythonic way to go about return values - but returning two objects, one for |
FYI, I've resolved the main issue for this in commit feeaf98, but I'm going to leave this open for continued discussion. |
I just closed this in farmOS/farmOS.py#15. I added the |
I hadn't, no. I'll try to get this implemented here as soon as I can.
Ah, that does make sense. |
Closing in favor of 2.x development. |
Right now, the library is using two separate functions under the hood for GET requests:
request()
, which is used if a page # is passed as a filter, andrequestAll()
.request()
simply returns the raw response from the server, without modification. So the response looks like:However,
requestAll()
loops through all the possible pages and concatenates the results onto a simple array, which is ultimately the return value of the function. The response looks like this:There's definitely an argument to be made that the two responses should be consistent, regardless of whether the results are paginated or not. See farmOS/field-kit#198 (comment). Another question is how much of the response should we try to reproduce, even if we are concatenating results. Do we include the
self
,first
, andlast
properties too?@mstenta & @paul121, I'm curious what your thoughts are on how the response should be formatted. I feel like we want consistency across implementations, as well as internal to each library. How is this being handled currently in farmOS.py?
See farmOS/farmOS.py#4 for a similar discussion of standarizing return types for different method calls.
The text was updated successfully, but these errors were encountered: