search

goshippo / shippo-python-client

Shipping API Python library (USPS, FedEx, UPS and more)
https://goshippo.com/docs
MIT License
123 stars 70 forks source link

Support for python 3.6+: deprecates the 'async' keyword argument (now… #47

Closed kurtiss closed 5 years ago

kurtiss commented 6 years ago

Support for python 3.6+: deprecates the 'async' keyword argument (now a SyntaxError), in favor of 'asynchronous'. Updates docs, examples, tests.

alephyud commented 5 years ago

Is there any chance that this pull request will be merged?

revmischa commented 5 years ago

merge it please!

adamszeptycki commented 5 years ago

Would be nice to have it merged to master

malcolmrebughini commented 5 years ago

Thanks a lot for taking the time to make this PR and sorry it took so long to get it reviewed.

Code looks good, but sadly this introduces a bug.

The CreatableAPIResource class passes down all extra parameters as the body for requests. And the Shippo API takes on some endpoints the parameter async. So the client was actually relying on this parameter having this name for this behavior.

I confirmed this issue by running some examples and taking a look at the requests generated. Examples fail because async defaults to true server side if not present.

I'll probably merge this anyways and fix this issue on master in the next couple of days.

Thanks!

electric-monk commented 5 years ago

I was able to run the changes from #47 by adding

    if params is not None:
        params = {('async' if k == 'asynchronous' else k): v for k, v in params.items()}

into APIRequestor.request().

However, I also had a bunch of other except statements to fix, and a few other changed names that still seem to be present in #47 (changing import urlparse to import urllib.parse as urlparse and urllib.quote_plus to urllib.parse.quote_plus for example), so I'm not sure I actually started from the right branch/commit.

malcolmrebughini commented 5 years ago

I'll merge this into the branch feat/python-3.6-support to continue work in that branch.