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

PEP8, test fixing, code cleanup #5

Closed SteveByerly closed 8 years ago

SteveByerly commented 8 years ago

These changes clean up the codebase quite a bit and get the tests in a usable state.

I also removed the 'sync' param since the API already has a native 'async' param that can be used. I think using the native methods work better here since you don't have to rely on the python package to be updated to use them (e.g. there is no 'sync' on the Manifest resource).

I would suggest removing the manifest testing since Shippo has no way of generating test manifests, and this could lead to screwing up production data.

mnowik commented 8 years ago

Hi @SteveByerly , thanks for the PR and the time that you spent on it. Do you mind if I create a separate PR with your commit ac5d45b related to PEP8 to make it clearer ?

For the rest, I will run the tests locally and will merge it to master.

Cheers, Matt

SteveByerly commented 8 years ago

Hi @mnowik , I removed the debugger - sorry for leaving that in there. I didn't realize PRs were directly tied to branches and I guess my two PRs were really just one. Do you want me to split them up into two branches and resubmit?

mnowik commented 8 years ago

@SteveByerly if you don't mind creating 2 PRs. If you don't have the time, I can do it.

SteveByerly commented 8 years ago

@mnowik Wanted to see if there was any feedback/issues with the submitted PRs

mnowik commented 8 years ago

@SteveByerly didn't have the time to look at it more in details. I will do it this week, and will ping you when I merge.

SteveByerly commented 8 years ago

Any updates on these PRs?

mnowik commented 8 years ago

@SteveByerly sorry for the delay, I got distracted by another project. I created a helper to merge 190e93071a0c1a320ddc533c1a10babdfa68ef82 your PR and I started creating fixtures for our tests. I will do my best to finish this in the next week