support update paypal accounts & custom params for list operation #53
Conversation
wmews-hw
left a comment
wmews-hw
left a comment
There was a problem hiding this comment.
Hi @dfreudenberger,
We don't have a schedule defined for server SDKs. It is basically on-demand.
Can you please address my comments before we move forward?
| * @param params Custom filter options | ||
| * @return HyperwalletList of HyperwalletUser | ||
| */ | ||
| public HyperwalletList<HyperwalletUser> listUsers(HyperwalletPaginationOptions options, Map<String, String> params) { |
There was a problem hiding this comment.
Can you provide to me the use case so I can better understand why a generic params map is meant for?
There was a problem hiding this comment.
Sure. According to the documentation each "list endpoint" supports different query params to filter the result. I.e. the list users endpoint supports the email parameter while the list bank accounts endpoint supports the type parameter. In order to find a specific user the best / most performant way to find the user would be filtering by email address which is not supported by the current implementation. Listing all users and filtering client side seems like a huge waste of resources.
We could have well defined query classes for each endpoint only exposing the supported params. If it'd be up to me only I would go with the map implementation since it allows developers to use parameters that might have been introduced to the api but are not yet included in the the query classes.
There was a problem hiding this comment.
Following the design we have today, I'd prefer creating a HyperwalletUserListOptions that extends HyperwalletPaginationOptions.
I get when you say that this is way more flexible, but we want to keep our SDK self contained without requiring the developer to go check our documentation to understand what is available to be used and what isn't. That also helps us unit test it.
There was a problem hiding this comment.
Fine for me. I'll pick it up.
There was a problem hiding this comment.
Please check again. I'm not super happy with the current implementation but tried to prevent unnecessary changes.
My idea would be:
- introduce
getParametersintoHyperwalletPaginationOptions- to make this change happen, the date conversion needs to be moved from
HyperwallettoHyperwalletPaginationOptions
- to make this change happen, the date conversion needs to be moved from
HyperwalletUserListOptionswould overridegetParameters, take the parent params and add its ownpaginateoraddParametersinHyperwalletcould be replaced with one of the others
| return this; | ||
| } | ||
|
|
||
| public Map<String, String> getParameters() { |
There was a problem hiding this comment.
I did like @dfreudenberger but I will ask you to stick with the current design. We are planning to do changes on all our server SDKs to support the changes in our REST v4 and I will keep it in mind to implement for all resources.
|
@dfreudenberger please let me know if you can do that so we can merge and close this pull request. Again, we really appreciate your effort expading our capabilities. |
3 participants
Hi,
any chance to get the PR integrated into the master? How long does it usually take to get a new release out.
If more changes are required (i.e. support custom params for all list operations) please let me know.
Best,
Daniel