Added get_my_fees_estimate() to products.py#216
Conversation
Saw there wasn't a get my fees estimate. Function arguments must be lists but, leaves it open to make 20 requests all different to one another or, for example the same marketplace and currency but everything else different.
|
This is my first pull requests, just hoping I haven't messed anything up. So, I don't really understand the above. |
GriceTurrble
left a comment
GriceTurrble
left a comment
There was a problem hiding this comment.
Thank you for the PR, I know this method has been requested before and development stalled on it (my fault for being indecisive and changing priorities 😅 ).
As mentioned in the comments, the overall design here is a bit rough, relying on tightly-coupled lists that are intended to line up so that the details of different fees requests are packed together. This creates a code smell, indicating that a more object-oriented design pattern might be a better approach (see example commented around line 171). As such, we can't accept this change in its current form.
On a positive note, really appreciate the detailed docstring describing the design intent. More of that, please!
And side note, as mentioned in comments, we expect to merge in changes from #207 first, and there is a slight adjustment that will be needed to line up with the signature change to make_request once that is merged (see comment around line 202).
Thank you!
| try: | ||
| values_dict['MarketplaceId'] = marketplace_id[i] | ||
| except IndexError: | ||
| values_dict['MarketplaceId'] = marketplace_id[0] |
There was a problem hiding this comment.
I can see how this is attempting to use a list of values that contains either 1 or n items, where n is intended to be the same as the number of id_value elements. However, in the case that any given list is shorter than it should be, this will simply return the first element for each subsequent id_value:
id_value = [0, 1, 2, 3]
marketplace_id = ['a', 'b']The above would create sets like (0, 'a'), (1, 'b'), (2, 'a'), (3, 'a') (the last two re-using the first element in marketplace_id). This may create an error if the data in position 0 is not meant to match up with the data in position x of any other list. Trying to account for this error means checking list length for every list before processing.
That point is moot, however, as the design becomes too cumbersome to manage with these tightly-coupled list arguments (see other comments on review).
| } | ||
|
|
||
| values = [] | ||
| for i in range(len(id_value)): |
There was a problem hiding this comment.
As a general point, iterating using range(len(...)) is an anti-pattern. If the index is required in the loop, you're better off using enumerate():
for idx, val in enumerate(id_value):
print(marketplace_id[idx])
print(val)| ): | ||
|
|
||
| """ | ||
| Each argument must be a list, up to a length of 20, with id_value (number of ASINs or SellerSKUs) determining how many requests are made. |
There was a problem hiding this comment.
This design gets cumbersome to manage, as it requires every argument to be a list of exactly 1 or exactly n elements, where n is supposed to match the length of one of those list arguments (id_value). This cannot easily account for situations where any given list contains x number of elements where x != 1 and x != n.
Further, we can certainly account for the 20 request limit specified in Amazon's documentation, but current code is not performing that check.
I think we'd be better served if each item is defined by a dictionary, like so:
fee_requests = [
{
'id_value': 'B082YTKC47',
'id_type': 'ASIN',
'marketplace': 'A1F83G8C2ARO7P',
'is_amazon_fulfilled': True,
'identifier': 'Fee requests',
'listing_price': {
'amount': 19.0,
'currency_code': 'GBP',
},
'shipping': {
'amount': 5.0,
'currency_code': 'USD',
},
'points': {
'number': 5,
'monetary_value': {
'amount': 2.0,
'currency_code': 'EUR',
},
}
},
# and so on, up to 20
]The request method should then be able to accept one of the following signatures:
# a single list argument, containing a list of dicts:
resp = products_api.get_my_fees_estimate(fee_requests=fee_requests)
# OR an arbitrary set of arguments that can be unpacked, each one being a dict:
resp = products_api.get_my_fees_estimate(*fee_requests)I'm a little partial the second one myself, but either will do. 🤷
There was a problem hiding this comment.
Note that the example also includes shipping and points, which can also be requested according to documentation (see PriceToEstimateFees in the Products Datatypes doc). Full coverage of the request's potential parameters will be necessary before the method can be accepted.
| data = { | ||
| "Action": "GetMyFeesEstimate", | ||
| } |
There was a problem hiding this comment.
Minor point, an upcoming change (#207) will move the Action parameter to the signature for make_request. I recommend removing this key from data, and then at the end of the method calling:
return self.make_request("GetMyFeesEstimate", data)| Options: | ||
| A single element list which repeats, or | ||
| A list of equal length as id_value for a one to one relationship | ||
|
|
||
| Example: | ||
| marketplace_id = ['A1F83G8C2ARO7P'] | ||
| id_value = ['B082YTKC47', 'B07WGJ9JGP', 'B01LXP5TXI'] | ||
| id_type = ['ASIN'] | ||
| is_amazon_fulfilled = [True, False, True] | ||
| identifier = ['request1', 'request2', 'request3'] | ||
| listing_price = [9, 16.42, 2.99] | ||
| listing_price_currency_code = ['GBP'] | ||
|
|
||
| Example 2: | ||
| marketplace_id = ['A1F83G8C2ARO7P', 'ATVPDKIKX0DER', 'A13V1IB3VIYZZH'] | ||
| id_value = ['B082YTKC47', 'B07WGJ9JGP', 'B01-LXP-TXI6'] | ||
| id_type = ['ASIN', 'ASIN', 'SellerSKU'] | ||
| is_amazon_fulfilled = [True] | ||
| identifier = ['Fee requests'] | ||
| listing_price = [19] | ||
| listing_price_currency_code = ['GBP', 'USD', 'EUR'] | ||
|
|
||
|
|
||
| Returns Amazon Fees for ASINs and SellerSKUs specified. | ||
|
|
||
| Docs: | ||
| https://docs.developer.amazonservices.com/en_UK/products/Products_GetMyFeesEstimate.html |
There was a problem hiding this comment.
Despite the points made about the overall design, this is great documentation to explain the method's usage and options. Really appreciate having the docstring be this detailed to help me see the intended design. 👍
No worries, not much to be messed up here. The Codecov report above is simply indicating the lack of a test for the new code. We would appreciate a test being added that at least covers the expected request parameters. See the code in Note there are also checks in place for code formatting using On that note, I may need to ask you to sync up with |
Just to re-iterate what @GriceTurrble has said - thanks for the PR it is appreciated, and don't let the review put you off from contributing. A bit of a nudge here and there in your code and it will happily get merged in! Keep up the good work. |
|
@Ryan-Daly Major updates in #207 have been merged in, and this PR is now out of date with those. As such, I think closing this PR is for the best. Please pull latest from Thank you for the contribution, and I hope to see you come back with more in the future. |
|
@GriceTurrble Thanks for the feedback! Really appreciate it how in depth you went. Looks like someone else has this method covered now but I'll definitely contribute more in the future. I'm using the library so I'll help build it when I can. |
3 participants
Saw there wasn't a get my fees estimate. Function arguments must be lists but, leaves it open to make 20 requests all different to one another or, for example the same marketplace and currency but everything else different.