sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Setup consistent way for optional parameters (like `page` and `perpage`) through all the SDK functions

Open grafov opened this issue 5 years ago • 2 comments

It is about other ways to solve the problem fixed in #46.

There are a lot of functions in Grafana API that could accept page and perpage as optional params. In a number of functions, there is also query parameter. I think it would be good for clarity to setup them all in a consistent way through all the SDK. It could be reached in a number of ways:

  1. Add page and pergage explicitly to the functions where it applicable. Additionally, define the functions with All suffix like GetAllUsers that would get whole object lists without paging parameters.
  2. Setup page and perpage as optional slice like that: GetUsers(paging ...int). It may be not obvious though and requires a careful explanation in the function comments.
  3. Pass optional functional parameters to all the functions of the SDK where optional params allowed. So it would allow pass them in any order:
sdk.GetUsers()
sdk.GetUsers(sdk.Page(200))
sdk.GetUsers(sdk.Query("match"), sdk.Page(1), sdk.Perpage(10))

It could be refactored for the next major release with breaking changes. So the current state could be fixed with v1.0 (it has 0.x numeration yet). And the change proposed above marked with v2.0

I don't decide that variant would be better for the API clarity yet. The latest way with funcparams looks more attractive for me.

grafov avatar Nov 03 '19 16:11 grafov

It is about other ways to solve the problem fixed in #46.

There are a lot of functions in Grafana API that could accept page and perpage as optional params. In a number of functions, there is also query parameter. I think it would be good for clarity to setup them all in a consistent way through all the SDK. It could be reached in a number of ways:

  1. Add page and pergage explicitly to the functions where it applicable. Additionally, define the functions with All suffix like GetAllUsers that would get whole object lists without paging parameters.
  2. Setup page and perpage as optional slice like that: GetUsers(paging ...int). It may be not obvious though and requires a careful explanation in the function comments.
  3. Pass optional functional parameters to all the functions of the SDK where optional params allowed. So it would allow pass them in any order:
sdk.GetUsers()
sdk.GetUsers(sdk.Page(200))
sdk.GetUsers(sdk.Query("match"), sdk.Page(1), sdk.Perpage(10))

It could be refactored for the next major release with breaking changes. So the current state could be fixed with v1.0 (it has 0.x numeration yet). And the change proposed above marked with v2.0

I don't decide that variant would be better for the API clarity yet. The latest way with funcparams looks more attractive for me.

I think the third option looks the best out of these as well. However, not sure that we should release v1.0 with the current code because it might not work in cases where someone might have lots of users, for example (https://github.com/grafana-tools/sdk/pull/46). IMO we should opt for correctness here before v1.0.

GiedriusS avatar Jan 20 '20 19:01 GiedriusS

Well, there is no deadline for the release here (for me anyway :) so I agree. Let make API consistent before v1.0 appeared. 0.x numeration assumes that API not stable yet.

grafov avatar Jan 22 '20 13:01 grafov