`boto.mws` - various fixes and code cleanup #1416

Open
wants to merge 7 commits into
from

4 participants

@prmtl

This pull request is an attempt to make code of the boto.mws more readable and adding some small tweaks and fixes.
So far:

  • added funtools.wraps decorator
  • cleaned code of require decorator
  • fixed MWSConnection.create_fulfillment_order
  • fixed ListInventorySupplyResult
  • ResponseFactory now can check if call results class is already defined (moved that feature from api_method decorator

I'm planning to add more tests (unittests, integration tests).

Please do not merge it - I'm just wanted to get some feedback from other boto developers.

prmtl added some commits Mar 28, 2013
@prmtl prmtl fixed decorators for create_fulfillment_order 5936068
@prmtl prmtl Fixed ListInventorySupplyResult 8613711
@prmtl prmtl added `requires_v2` decorator in boto.mws
`requires_v2` is a better verion of `requires` - it shows which params are missing from decorated method instead of listing all params
it has also a more cleaner code (easier to read)
all methods should use it instead (tests required)
a4fa6c5
@prmtl prmtl added functools.wraps decorator to do all the fancy stuff when decora…
…ting function
373d12d
@prmtl prmtl replaced old requires decorator with new one and fixed issue with rai…
…sing error when at least one group has been completed
b956aba
@prmtl prmtl better way to utilize ResponseFactory 2810b5d
@disruptek

Can you elaborate on the goal of this commit?

You can see example how @wraps works here: http://docs.python.org/2/library/functools.html#functools.wraps

@wraps make the wrapper to look like the wrapped function. This involves copying/updating a bunch of the double underscore attributes—specifically module, name, doc, and dict
Small example:
Before:

>>> MWSConnection.create_fulfillment_order.__name__
'wrapper'

After using @wraps:

>>> MWSConnection.create_fulfillment_order.__name__
'create_fulfillment_order'

Sure; I was just looking for some excuse for the addition of code and wanted to make sure it was useful to the rest of the module. Though it's not, yet, I can see how your example is a legitimate use-case. :-)

@disruptek

Good, thank you.

@disruptek

Looks like I was way off on this one, thanks for the fix.

@disruptek

Seems worthwhile, but it might make more sense to instead output Sphinx markup from the decorators.

👍

@disruptek

Okay, but I think we want to search for a predefined Result class in any case, no? Maybe that should be performed by the Response class; then it can be extended and overridden with more obvious mechanics.

Okay, but I think we want to search for a predefined Result class in any case, no?

And this is how it works now (and before).

Maybe that should be performed by the Response class; then it can be extended and overridden with more obvious mechanics.

Good idea. I'll check it.

@toastdriven

Overall, +1. I think most of this would have an easier time getting in as a handful of smaller, focused PRs (as opposed to one large PR).

@disruptek

So, what happened here? Did the documentation change and/or does it not reflect what you're receiving on the wire? It seems strange that NotificationEmailList does not occur elsewhere...

Do we need to define the FulfillmentShipmentPackage and FulfillmentOrderItem classes? I know I've sometimes defined these "empty" response classes, perhaps for readability (and in any event, inconsistently). But can we not omit that code and simply issue a FulfillmentShipment = MemberList() for brevity and simplicity?

My goal with the scaffolding in the response parser was to make each response definition as succinct and legible as possible, which seems important to me given that we already have ~60 different responses. To that end, I hope that we can define as little as possible without hindering comprehension on the part of the reader.

By the way, I appreciate the fact that you're paving the fulfillment road for me here. I'm hoping to finally take advantage of my (and your) code for these calls later this year, so each commit you make is saving me some future debugging. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment