Improve semantics for listing methods #569
Conversation
|
Is the idea that the Rather than supporting a global limit, we should instead switch to the lazy iterator approach and allow users to pull as many resources as they need. |
|
@mgyucht https://github.com/databricks/databricks-sdk-go/blob/main/.codegen/api.go.tmpl#L219-L221 i think we need to change this to |
|
Once I get around to #543, users can define their own iterators and filter however they like. IMO I don't think it needs to be built into the Go SDK, and it isn't in any other SDK we publish today. |
|
@mgyucht it's orthogonal to #543 (we should hold on that because of the upcoming language changes - golang/go#61405). this change is about cross-SDK semantics on removing low-level fields to paginated methods, so it should be merged ;) |
This PR moves listing requests to an `ListXXXInternal` entity, which keeps all the low-level pagination request fields, like `offset`, `limit`, `token`, `page`. Original entity keeps all of the fields, except pagination-related. This allows to mitigate a common confusion, where users supply `limit` for the iterator and it acting only a _result page limit_, not the _max number of results_.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 17.97% 18.04% +0.07%
==========================================
Files 85 85
Lines 9805 9990 +185
==========================================
+ Hits 1762 1803 +41
- Misses 7889 8030 +141
- Partials 154 157 +3
☔ View full report in Codecov by Sentry. |
| if svc.Name == "Jobs" { | ||
| // add generation for client-side maximum number of results during iteration. | ||
| // platform already implements this field for part of the APIs and it's | ||
| // consistently named as max_results everywhere. Perhaps we should |
There was a problem hiding this comment.
max_results is also confusing and incorrect as listing will return all results, regardless of the value passed here. Can we rename this field to page_size instead? This aligns with our internal standards on pagination.
There was a problem hiding this comment.
it we use any other name than limit, it'll break the CLI. @pietern are we fine with that? :)
max_results is also confusing and incorrect as listing will return all results, regardless of the value passed here.
we can make deterministic logic here. max_results is really the closest thing semantically. page_size is not applicable here, as this listing request is for the entire dataset, not the page. the concept of page is totally hidden from a user.
There was a problem hiding this comment.
If the concept of page is hidden from users, then we should not expose this parameter at all, as its only use is to determine the page size.
There was a problem hiding this comment.
we're keeping this limit for CLI, then. other renames are out of scope for now.
|
Can we prepare the changes in the other SDKs at the same time, since we're making changes to the template logic? This change seems good to me, but to minimize clashes with others developing on the SDKs, I want to decrease the wall-clock time that the OpenAPI generator won't work with the other SDKs. |
This PR moves listing requests to an
ListXXXInternalentity, which keeps all the low-level pagination request fields, likeoffset,limit,token,page. Original entity keeps all of the fields, except pagination-related. This allows to mitigate a common confusion, where users supplylimitfor the iterator and it acting only a result page limit, not the max number of results.Follow-up Python SDK PR: databricks/databricks-sdk-py#279