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

Resource refactor #7

Closed SteveByerly closed 3 years ago

SteveByerly commented 8 years ago

There was a bit of unnecessary repetition in the resources around generating urls for api calls. It also seems to make sense to set the API version in one place, and use a variable, instead of hard-coding it into the url strings.

I also figured it made sense to make resources produce objects of the resource type, instead of having everything return ShippoObject objects. This way I could wire something up like 'refresh' so you don't need to necessarily re-retrieve an object to update its values. Same thing with 'get_rates' for a shipment - you should be able to get the rates by asking the instance for them directly.

I'm not sure the 'api_key' needs to be passed around as much as it does, but I figure I would leave that for another time and wait to see if there's a need for it.

I piggy backed this branch onto the other PEP8 changes so I wouldn't have to work around formatting and future merge conflicts that would need to be resolved - that is if you accept the original PR.

I couldn't really test all these changes since the tests were already broken. The tests should be fixed in my other PR, so I can address broken tests if those changes get accepted.

mnowik commented 8 years ago

@SteveByerly Tks for the PRs. I am merging them right now into a helper branch. I will fix the tests and will deploy over the weekend.

SteveByerly commented 8 years ago

Awesome! Sorry for not seeing your note and fixing the path.

Let me know if you have any feedback on the changes.

mnowik commented 8 years ago

@SteveByerly just to be sure. I started merging #5 in a branch-helper. I also need to merge #7 after right ?

SteveByerly commented 8 years ago

Yeah, #7 could be merged in afterwards. I didn't touch the tests on this PR because there were so many changes to the tests in #5 that I didn't want to duplicate or clobber anything, while keeping the scope of the changes separate.

After you merge, I'd be happy to go back in and update any tests that no longer pass.