clean method#65
clean method#65GriceTurrble merged 22 commits intopython-amazon-mws:developpython-amazon-mws/python-amazon-mws:developfrom elcolumbio:developCopy head branch name to clipboard
Conversation
GriceTurrble
left a comment
There was a problem hiding this comment.
I think there are some good points here, but there are a few details missing that I'd like addressed, and a slight refactor in terms of the try..except block being used.
Aside from that, I think we can keep building on this to improve input cleanup and prevent a lot of common input mistakes.
mws/mws.py
Outdated
| @@ -250,10 +274,7 @@ def make_request(self, extra_data, method="GET", **kwargs): | ||
| # Amazon's MWS does not allow such a thing. | ||
| extra_data = remove_empty(extra_data) |
There was a problem hiding this comment.
I missed this part earlier, but I don't see a point to having two lines to command operations on the same object. We can tuck the "remove_empty" call (or the code itself, since it's a one-liner) into the "clean_extra_data" method.
mws/mws.py
Outdated
| value = str(value) | ||
| try: | ||
| value.lower() + value + '' | ||
| except: |
There was a problem hiding this comment.
flake8 fails the build due to a bare except, and then the test suite won't run at all. Aside from that, the test case for this exception is running .lower() and concatenating with +? Seems like an odd implementation.
Per another comment (regarding isinstance(value, (bool, int))), I think the proper try test is simply value = str(value), but it's highly unlikely that that will even throw an exception of any kind. Thus, I don't know if a try...except here is even necessary.
mws/mws.py
Outdated
| for key, value in extra_data.items(): | ||
| if isinstance(value, (datetime.datetime, datetime.date)): | ||
| value = str(value.isoformat()) | ||
| if isinstance(value, (bool, int)): |
There was a problem hiding this comment.
I think we only really need to check for bool here, since we'd like to form the boolean param to its lowercase equivalent (that looks like what MWS expects for requests). I would suggest:
if isinstance(value, bool):
value = str(value).lower()The other portion for checking an int is mostly irrelevant. A later code line, ideally, would just call str over all values, which would naturally handle this case.
Aside from this, I think we should add a condition at the top of the method that checks for specific data types we know won't work, like dict, list, set, and tuple. If we raise an exception for those types, we can clear up a lot of potential confusion early on.
mws/mws.py
Outdated
| extra_data_enc = dict() | ||
| for key, value in extra_data.items(): | ||
| if isinstance(value, (datetime.datetime, datetime.date)): | ||
| value = str(value.isoformat()) |
There was a problem hiding this comment.
The str() conversion isn't necessary: isoformat outputs a string in all cases.
| self.assertEqual(params['AvailableFromDate'], from_date.isoformat()) | ||
| self.assertEqual(params['AvailableToDate'], to_date.isoformat()) | ||
| self.assertEqual(params['MaxCount'], max_count) | ||
| self.assertEqual(params['Acknowledged'], str(acknowledged)) |
There was a problem hiding this comment.
Given other suggestions for converting a bool to either "true" or "false", this and other parameters that use booleans would need a slight update to match (sorry for the nitpick :) ).
mws/mws.py
Outdated
| response = None | ||
|
|
||
|
|
||
| class InputError(Exception): |
There was a problem hiding this comment.
No big issue with adding a new Exception type, but we should be cautious about it. It might be better to simply raise ValueError in cases where this type is being used, or to overwrite it as, for instance, MWSValueError. Just a thought.
There was a problem hiding this comment.
good point. Now it's a very unlikely event, since we do less validation and more auto cleanup. Will remove it.
|
Note that this PR relates to #56. |
* Feeds example. * Feeds example. Use of ".parsed" changed.
|
thanks for your big help. I will come up with a new solution. |
Codecov Report
@@ Coverage Diff @@
## develop #65 +/- ##
==========================================
+ Coverage 75.14% 75.4% +0.25%
==========================================
Files 18 18
Lines 869 870 +1
Branches 91 92 +1
==========================================
+ Hits 653 656 +3
+ Misses 208 207 -1
+ Partials 8 7 -1
Continue to review full report at Codecov.
|
|
I guess we have two sorts of parameters:
|
also it's helpful to understand how we modify params
|
Sorry, this became messy. I think this should be the first commit to look at. I did only fix tests which were failing. So if you e.g. change a parameter like 'nospace' to 'no space'. |
GriceTurrble
left a comment
There was a problem hiding this comment.
Slight move requested for one code line. That move could change the output slightly, so need some testing to ensure the common request params (get_default_params()) are cleaned properly, and to see if the common test suite parses those defaults correctly.
Sounds like a lot, probably only a few line changes.
Good work on prior changes, though. The method itself looks good, just need to try it out. :)
mws/mws.py
Outdated
| "bar=4&baz=potato&foo=1" | ||
| """ | ||
| description_items = [] | ||
| params = clean_extra_data(params) |
There was a problem hiding this comment.
This is called twice at the moment (see other note).
This call can be removed. I think to properly test param outputs, we need to keep the other call, but move it slightly.
mws/mws.py
Outdated
| for key, value in extra_data.items(): | ||
| if isinstance(value, (datetime.datetime, datetime.date)): | ||
| extra_data[key] = value.isoformat() | ||
| extra_data = clean_extra_data(extra_data) |
There was a problem hiding this comment.
This is called twice at the moment (see other note).
As noted, this feels like the correct place to call the method, but since it is not including get_default_params() or get_proxies(), it might be missing stuff.
I suggest moving this call to the line just above if self._test_request_params. That way our test methods capture every param, all quoted and cleaned up, ready to transmit.
That may require a change to the common test params, as well. We'll just have to try it out.
There was a problem hiding this comment.
Note with this change it would need to clean params, not extra_data. Just to clarify.
There was a problem hiding this comment.
yes thank you so much. I think you nailed it, it's like that.
see idea from GriceTurrble in this pull request
|
Tried out a few request methods. They all seem to work :) |
no need for this
|
I commented it in the issue |
This reverts commit 2a18583.
|
i guess i shouldn't merge other pull request. I don't want to change anything in the docs. Sorry for the mess. |
| return [x for x in seq if not (x in seen or seen_add(x))] | ||
|
|
||
|
|
||
| def dt_iso_or_none(dt_obj): |
There was a problem hiding this comment.
Removing this method is a fair change. I added it myself from a version of the API I was using in production years ago, but under current circumstances it doesn't serve much of a purpose.
While this method did provide some enforcement requiring a user to pass a datetime to a request method (and get an early exception if they didn't), I think that's an acceptable risk for the time being. We could also offer type hints ala PEP 484, but that is a topic for another day.
adjust docs, we moved functionality list comprehension seems nicer
* Release for 0.8.0 (python-amazon-mws#42) * Added Finances API Feature * .setup.py: bump version to 0.7.5-dev0 * Split out request_description building from make_request() So that it can be tested more easily, and refactored * Split out building the initial params dict from make_request() So that it can be tested more easily * Add fake MWS credentials pytest fixture * test_service_status() should use the pytest fake credentials fixture * Add more pytest fixtures (access_key, secret_key, account_id, timestamp) * Add test for calc_request_description() * Split out calc_request_description() into more statements So that it is easier to debug * Fix calc_request_description - don't include leading ampersand * Don't do automated deployments via Travis (for the moment) * Update README.md badges * InboundShipments, next_token_action decorator, and some style cleanups. (python-amazon-mws#33) * Testing out git commits from VS Code * Reverting the test commit * Adding VS Code settings to gitignore. * Style fixes * MWS.enumerate_param deprecated: now using utils.enumerate_param and utils.enumerate_params * InboundShipments fleshed out; added `utils.next_token_action` decorator; deprecated separate methods for `...by_next_token()` * Bugfix, rename `_enumerate_param` to `enumerate_param` (no need for private) * Fix for next_token issues. * TravisCI flake8 complaining, fixes. * Minor flake8 complaint. * Hack to get flake8 to stop complaining. * Strip pylint disables to clear line length issue. * Correction to keyed params, now tests every item in values sequence to ensure all are dicts. * Add tests for param methods in utils. * Add test for next token decorator. * Adding 'InboundShipments' to `__all__` * Assigning response a default in __init__ for DictWrapper and DataWrapper * Unneeded line breaks removed + docstring formatting * Comment corrected. They're tuples, not sets. * Finances methods updated to use next_token_action decorator * Create .travis.yaml * Update .gitignore * Removing deploy code from local travis * Delete .travis.yaml * Pushed to 0.8.0-dev0 Recently added functionality to InboundShipments, as well as Finances API. These constitute feature additions with backwards compatibility, which calls for a minor version update. * Adding Python 3.6 category We are testing in 3.6 in Travis anyway. May as well include the note. * Specified master and develop branches for badges Ensured the badges for Travis and Codecov are pointing to the appropriate branches (used to be pointing to default, which was master in both cases). * Updated comments throughout module No substantial code changes, comment changes only. Also ensured all docstrings follow same format. * Fixed docstring formatting Also made slight update to docstring for ObjectDict to more clearly note what it does, vs what the original code did. * Fix for flake8 (trailing whitespace) * Fix for flake8 (trailing whitespace) * Bump to 0.8.0 (drop dev tag) for release * Bug: Incorrect use of `super` for back-compat Using the old-style `super` syntax to comply with Python 2.7 compatibility. Not revealed in tests, because current tests don't touch the APIs. Whoops! * Added back old object names in case needed Old names `object_dict` and `xml2dict` added back in case the old objects are being used directly by some users. To be removed in 1.0.0 release down the road. * Version bump for agent string. * Hi, howyadoin? * Format request url with str.format Remove old-style string formatting for sake of clarity. * BUG: GetReportList missing from next token ops Added in with a hotfix. * Version push * version push * version bump (corrects wrong version from before) * Bugfix inbound constructor (0.8.3 release) (python-amazon-mws#57) * version bump * No from_address assignment needed in Inbound init Similar fix as commit on `develop`, which will come in with 1.0 release. Also, version bump for new bugfix release. * Remove import for ExpatError We do not support Python < 2.7, in which ParseError is available; import for ExpatError is not necessary. * Comments added for later work. * Added '.' at end of some params to test Params ought to pass through `enumerate_keyed_param` whether they end with '.' or not. * Added test for dict_keyed_params * Added test for enumerate_keyed_param Also fix the existing one (wrong info used) * Convert test methods to testcase class * Create example_response.txt File contains an example response from MWS, taken directly from MWS documentation. To be used for testing utilities. * Change test methods to test case classes * Add `download_url` to setup.py * Fix issue python-amazon-mws#60 (python-amazon-mws#61) * Fix issue#1 Always return an interable list. Stolen from https://github.com/bloodywing Thanks! * Fix issue python-amazon-mws#60 We must sort the params before encoding and concatenating or we will fail our calls. This is a nice hybrid between the old version and a nicer more pythonic way of doing things with .join and not just appending to strings and then hacking the last ampersand off. * Sorting of dict keys instead of the dict object Essentially the same output, slightly more explicit for an easier time reading it later. * Feeds example. (python-amazon-mws#64) * Feeds example. Use of ".parsed" changed. (python-amazon-mws#66) * Feeds example. * Feeds example. Use of ".parsed" changed. * Subscriptions api (python-amazon-mws#67) * Fix issue#1 Always return an interable list. Stolen from https://github.com/bloodywing Thanks! * Fix issue python-amazon-mws#60 We must sort the params before encoding and concatenating or we will fail our calls. This is a nice hybrid between the old version and a nicer more pythonic way of doing things with .join and not just appending to strings and then hacking the last ampersand off. * Sorting of dict keys instead of the dict object Essentially the same output, slightly more explicit for an easier time reading it later. * Start working on the subscriptions API * More updates to subscription to test. * missed off Subscription * remove subscription, example appears to not require this? * subscription required for subscription calls, not on destination, * flake8 make creation of call consistent accross this file. * indentation error. * Subscriptions (#9) * Fix delete subscription * Fix delete subscription * Subscriptions (#10) * Fix delete subscription * Fix delete subscription * _type -> notification_type * small doc update * Subscriptions (#11) * Fix delete subscription * Fix delete subscription * _type -> notification_type * small doc update * change subscriptions attributes_list to be attributes (dictionary) then map the key and value into the correct style for enumeration * Subscriptions (#12) * Fix delete subscription * Fix delete subscription * _type -> notification_type * small doc update * change subscriptions attributes_list to be attributes (dictionary) then map the key and value into the correct style for enumeration * flake8 after merge issues * Subscriptions (#13) * Fix delete subscription * Fix delete subscription * _type -> notification_type * small doc update * change subscriptions attributes_list to be attributes (dictionary) then map the key and value into the correct style for enumeration * flake8 after merge issues * missed off subscriptions in updated params * flake8 * fix get_subscription * move delivery_channel to the end of the params so it can be a default * clean method (python-amazon-mws#65) * clean input parameter * Feeds example. Use of ".parsed" changed. (python-amazon-mws#66) * Feeds example. * Feeds example. Use of ".parsed" changed. * see conversation pull request * modified tests to fit new clean method * revert start in subapi * git ignore * d * r * travis bugfix * call clean method for params too * reuse modifiers for request params also it's helpful to understand how we modify params * param test for wrong datatype exception * clean all parameters at once see idea from GriceTurrble in this pull request * remove dt_iso_or_none no need for this * Feeds example. Use of ".parsed" changed. (python-amazon-mws#66) * Feeds example. * Feeds example. Use of ".parsed" changed. * Revert "Feeds example. Use of ".parsed" changed. (python-amazon-mws#66)" This reverts commit ba0f363. * Revert "Feeds example. Use of ".parsed" changed. (python-amazon-mws#66)" This reverts commit e87012b. * Revert "Feeds example. Use of ".parsed" changed. (python-amazon-mws#66)" This reverts commit 77d3ea7. * Revert "Feeds example. (python-amazon-mws#64)" This reverts commit 2a18583. * Added support for zip files. * ignore files * Zip files downloaded to current directory. * Added unzip() function in DataWrapper object. * Minor updates, correcting my own mistakes from recent merge * Remove "assert_no_token". Not used except in a single test. * Fallback to _response_dict if _rootkey is not present in response dict (python-amazon-mws#72) This is the case while fetching some of the reports. I particularly found this in '_GET_XML_RETURNS_DATA_BY_RETURN_DATE_'. In this case the data is returned but not in rootkey. This will at least let the user parse the data on there own * Move flake8 checking after pytest calls A current PR is using `enum`, which was added in Py3.4. That should have produced an error for a missing module in Py2.7 testing, but `flake8` loads `enum34` as a dependency into the environment first. This contaminates the test suite prior to running tests, hiding what should be an error. * report enum and get_reportid (python-amazon-mws#74) * report enum and get_reportid * removed tuples since we do now handle the decoding better * removed get_request id and lets see if the test now fails * Move flake8 checking after pytest calls A current PR is using `enum`, which was added in Py3.4. That should have produced an error for a missing module in Py2.7 testing, but `flake8` loads `enum34` as a dependency into the environment first. This contaminates the test suite prior to running tests, hiding what should be an error. * extra require python2.7 enum34 * Streamline package requirements for enum34 * Bugfix, my mistake. Import setuptools needed * ... and sys * Push dev version num We've gone several versions up, technically, without changing the version num to match. May as well start somewhere. * Dev version num push (match setup) * Call to requests.request needs explicit timeout (python-amazon-mws#87) * Call to requests.request needs explicit timeout Call to requests.request doesn't explicitly mention timeout, without which could lead to zombie processes as default timeout is infinity (None). * Add marketplace ID enums #19 (python-amazon-mws#84) * enum marketplaces * typos * removed domain parameter, pls use region now, see MARKETPLACES * very basic tests * python-amazon-mws#91 add BR and AU and sort alphabetical * single quotes * better name for variable * idiomatic names, thanks to jameshiew * Changed excludeme default to be xsd1.1 compatible (python-amazon-mws#97) Occasionally, the MWS API for Products will throw an error, citing a non-xsd1.1 boolean as an example. Making the default value for the keyword argument excludeme into "false" fixes this. * Revert "Merge branch 'develop' into develop" This reverts commit 1a15914, reversing changes made to a7886c7.
Currently, the `0.8.x` branch breaks string parameters by encoding them twice, first in the `clean_params_dict` function, and then in `calc_request_description`. Because of this, parameters prone to have characters that must be escaped, like `NextToken` and URLs, do not work correctly. #258 was an attempt to fix this issue, but only for timestamp fields. Strings are still broken. This solution is a backport of a commit (#65) that is already present in the `develop` branch, where the `calc_request_description` function is no longer responsible for escaping the received parameters. It also rollbacks #258, as `clean_date` must encode the received values now.
first uncomplete version
to do: change all tests.