ej2 / python-quickbooks

A Python library for accessing the Quickbooks API.
MIT License
394 stars 193 forks source link

Remove simplejson #358

Closed pablogamboa closed 1 month ago

pablogamboa commented 3 months ago

This PR removes simplejson from python-quickbooks. It makes no sense to depend on simplejson in 2024. Why? The py3 stdlib json module is simplejson. Further the stock json module is faster than pypi's simplejson (which was its main appeal back in the py2 days).

The really annoying part is that if you install simplejson that's going to make requests automatically use it. When requests uses pypi simplejson it affects the handling of HTTP exceptions. This will unavoidably break a good few dozens of tests in any relatively large codebase (see related issue from another python-quickbooks user).

I believe that less is more in this case and python-quickbooks would be better off without simplejson. Heck, if you really want to add a nice json encoder/decoder then I would depend on orjson instead, but definitely NOT simplejson in 2024.

laf-rge commented 3 months ago

Conflict with https://github.com/ej2/python-quickbooks/pull/356. I also wonder what circumstances JSON parse speed would dominate over the network delay typical of the Quickbooks API. Do you have any performance data from your use case?

For my related pull request I'll explore if orjson has easy Decimal support.

laf-rge commented 3 months ago

I've updated my pull request #356 to include removing simplejson as a dependency as well. I think my pull request is now compatable with this one.

justmobilize commented 3 months ago

Just to throw in my 2 cents, since I agree with both of you: using native methods and having decimal precision:

I would add a param on init for use_decimal which defaults to False. Defaulting to True, could break existing users code that expects a float.

If it's True, then in the json loads, pass in parse_float=decimal.Decimal

Best of both worlds...

laf-rge commented 3 months ago

I've updated #356 with your suggestion

justmobilize commented 3 months ago

@laf-rge, I would still make the decimal usage an option. Otherwise this would be a breaking change

justmobilize commented 3 months ago

Oh you're too fast!

pablogamboa commented 3 months ago

Conflict with #356. I also wonder what circumstances JSON parse speed would dominate over the network delay typical of the Quickbooks API. Do you have any performance data from your use case?

What I mentioned in the PR description is that depending on simplejson made sense back in the py2 days, where the stdlib json was slower than simplejson. In py3, performance is not a reason anymore. And since performance is not a pro anymore, depending on simplejson is all cons now.

laf-rge commented 3 months ago

Conflict with #356. I also wonder what circumstances JSON parse speed would dominate over the network delay typical of the Quickbooks API. Do you have any performance data from your use case?

What I mentioned in the PR description is that depending on simplejson made sense back in the py2 days, where the stdlib json was slower than simplejson. In py3, performance is not a reason anymore. And since performance is not a pro anymore, depending on simplejson is all cons now.

The only other reason was Decimal and now with the changes in #356 we don't even need it for that.

pablogamboa commented 2 months ago

@ej2 what do you think about this PR and the related https://github.com/ej2/python-quickbooks/pull/356 ?

laf-rge commented 2 months ago

Happy for any feedback we probably need to update some documentation.