Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove simplejson #358

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Remove simplejson #358

merged 2 commits into from
Jul 9, 2024

Conversation

pablogamboa
Copy link
Contributor

@pablogamboa pablogamboa commented May 30, 2024

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.

@pablogamboa pablogamboa mentioned this pull request May 30, 2024
@laf-rge
Copy link
Contributor

laf-rge commented Jun 1, 2024

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?

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

laf-rge added a commit to laf-rge/python-quickbooks that referenced this pull request Jun 1, 2024
@laf-rge
Copy link
Contributor

laf-rge commented Jun 1, 2024

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

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 added a commit to laf-rge/python-quickbooks that referenced this pull request Jun 1, 2024
@laf-rge
Copy link
Contributor

laf-rge commented Jun 1, 2024

I've updated #356 with your suggestion

@justmobilize
Copy link

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

@justmobilize
Copy link

Oh you're too fast!

@pablogamboa
Copy link
Contributor Author

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
Copy link
Contributor

laf-rge commented Jun 3, 2024

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
Copy link
Contributor Author

@ej2 what do you think about this PR and the related #356 ?

@laf-rge
Copy link
Contributor

laf-rge commented Jun 27, 2024

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

@ej2 ej2 merged commit 3492ab1 into ej2:master Jul 9, 2024
6 checks passed
@pablogamboa pablogamboa deleted the pablogamboa/remove-simplejson branch July 10, 2024 16:13
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.

4 participants