Skip to content

Conversation

@gvisniuc
Copy link

@gvisniuc gvisniuc commented Nov 7, 2018

  • Added support for bulk API calls
  • Added support for beta API calls

Example usage:
beta - api.restv1(GET('financial/GLAccountClassificationMappings'), beta=True)
bulk - api.restv1(GET('Financial/TransactionLines'), bulk=True)

If you are routinely pulling all of the transactions, the bulk option greatly increases the speed of it since it fetches 1k per call.

@wdoekes
Copy link
Member

wdoekes commented Nov 7, 2018

Thanks for the PR!

So what are you saying? With the bulk API you don't need the iteration_limit option anymore?

Further: if x is True should be written as if x.

Additionally, this lacks documentation and a logical interface: if beta and bulk are true, then only bulk is used.

  • The bulk option looks clean enough to move to a separate BulkRest subclass which could be put in the right place in the API MRO. (It has nothing to do with the V1Division.)
  • But, what happens with the bulk option if you do something other than GET. Does it fail? Does it behave normally? Does it take different arguments?
  • As for the beta option: I cannot guess what it does, and as it's beta, the interface will possibly not be stable. So I'm very reluctant to add that. You can just Mix in your own ExactApi class if you really need beta in the mean time. (Replace V1Division with MyCustomBetaV1Division.)

@gvisniuc
Copy link
Author

gvisniuc commented Nov 8, 2018

@wdoekes
Hi,

Yes, that's true but it only works for a limited number of requests that have bulk support (Financial ones mostly).

Regarding the if x is True I wanted to explicitly check for the boolean True just to avoid setting the param to a string or other values that might evaluate as True.

  • Yes that sounds like a good option
  • It will most likely fail since BULK requests are GET only
  • Same, as 1, we can have a different class for that

@wdoekes wdoekes force-pushed the master branch 2 times, most recently from 45b794c to 9a44328 Compare January 27, 2020 13:59
@wdoekes wdoekes force-pushed the master branch 2 times, most recently from 32f519c to 24f97b5 Compare June 21, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants