search

bunq / sdk_csharp

C# SDK for bunq API
MIT License
35 stars 22 forks source link

Slight improvement of the PaymentListSample #32

Closed mikhail-denisenko closed 6 years ago

mikhail-denisenko commented 6 years ago

Rationale: I was quite confused with MONETARY_ACCOUNT_ITEM_ID in the beginning. Had to dig a bit deeper to figure out the MONETARY_ACCOUNT_ITEM_ID is actually MonetatyAccount -> MonetaryAccountBank -> Id I couldn't find it anywhere in the API documentation.

There's another sample that already features the possibility to query an account with a specific id so I thought it would be nice to have both approaches (hard-coded id and looping through all of the user accounts) showcased.

Corresponding PR: https://github.com/bunq/sdk_csharp/pull/31

OGKevin commented 6 years ago

As I said in this comment: https://github.com/bunq/sdk_csharp/pull/31#issuecomment-336613639

I don't see the need to introduce more logic for an endpoint that doesn't require it.

I couldn't find it anywhere in the API documentation. If this is the case then the issue is not SDK example related, but documentation related! If it was "better" documented, or easier for you to find what this ID was, then you could've ran this example without problems.

As for the loops, im not sure if we need to show how to loop over a list. Dont take me wrong but isn't this just too basic to show how to loop over a list and preform actions on the elements inside the list? The same actions we have show that you can do with a GET call you can do with a LIST call but then inside of a loop! But do we really need to show how to do such thing ?

So ultimately I suggest you create a together topic and suggest your improvements to the documentation regarding this information you couldn't find. As we want to keep the examples as simple and straight to the point without and extras.

What is your though on this @dnl-blkv ?

mikhail-denisenko commented 6 years ago

Yep, makes perfect sense - explaining the loop is a quite basic thing. However elaborating on Account ID would be great.

And a question from the PR conversation:

For future / upcoming PRs do you guys prefer to discuss it in the issue or the PR itself?

OGKevin commented 6 years ago

In the PR itself we like to discuss about the code. Which is the code review in most cases. For anything else we like to use the issue!

Regarding your question, have you read the documentation on this endpoint 😁 ? ATM we only have monetary accounts indeed, there is no such thing as a non-monetary account.

To get all your monetary accounts you can make a LIST call, inside the object of each account you will find its ID 👍. You can use this (MonetaryAccount) ID to make a GET call to just get that single MonetaryAccount.

OGKevin commented 6 years ago

Closing this issue, feel free to comment.