uptick / pymyob

A Python SDK for the MYOB Business (formerly AccountRight Live, and New Essentials) API.
BSD 3-Clause "New" or "Revised" License
18 stars 24 forks source link

Added two new endpoints and ability to handle query_string url keys/templates #76

Closed drewbroadley closed 1 year ago

drewbroadley commented 1 year ago

Added two new endpoints:

Improved handling for url keys/templates if there is a query string involved in endpoint.

jarekwg commented 1 year ago

Thanks for the contribution! Are those queryparams mandatory? I'm curious whether they belong in the endpoints definition, or whether they should be supported more how we support filters..

drewbroadley commented 1 year ago

For the Balance Sheet one, there's two of the three fields that are mandatory:

On Wed, 29 Mar 2023 at 13:30, Jarek Głowacki @.***> wrote:

Thanks for the contribution! Are those queryparams mandatory? I'm curious whether they belong in the endpoints definition, or whether they should be supported more how we support filters..

— Reply to this email directly, view it on GitHub https://github.com/uptick/pymyob/pull/76#issuecomment-1487779447, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALPUGB5CPGZZVXLZYOWSMTW6N7D7ANCNFSM6AAAAAAWKLXMIM . You are receiving this because you authored the thread.Message ID: @.***>

jarekwg commented 1 year ago

Lovely! Code looks great!

Queryparams in the endpoints file do make me a little uneasy, but given they're required, your solution is actually quite neat. We can always refactor in future to support queryparams better if more use cases were to pop up.

I also checked whether requests library is happy receiving queryparams in the url directly if there are others coming in via params, and it looks like it doesn't mind at all, and even composes them properly so that we don't get multiple ? in there:

In [1]: requests.get('https://www.google.com/?q=hello', params={'r': 'goodbye'}).request.url
Out[1]: 'https://www.google.com/?q=hello&r=goodbye'

Could you please add corresponding tests to the endpoints file (following the existing format), and then I'll go ahead and merge and publish!

PS: I also updated your opening comment to link to the myob api docs, just to see the queryparam stuff easier.

drewbroadley commented 1 year ago

Sorry for the silence. Yep, can write some tests this week and update PR.

jarekwg commented 1 year ago

Hey @drewbroadley I'm going to close this due to inactivity. Feel free to reopen if you get a moment to finish it off.