Skip to content

Added bulk api support and beta calls support#24

Open
gvisniuc wants to merge 2 commits into
ossobv:mainfrom
gvisniuc:v1-and-beta-api-support
Open

Added bulk api support and beta calls support#24
gvisniuc wants to merge 2 commits into
ossobv:mainfrom
gvisniuc:v1-and-beta-api-support

Conversation

@gvisniuc

@gvisniuc gvisniuc commented Nov 7, 2018

Copy link
Copy Markdown
  • 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

wdoekes commented Nov 7, 2018

Copy link
Copy Markdown
Member

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

gvisniuc commented Nov 8, 2018

Copy link
Copy Markdown
Author

@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