Fix MWS MalformedInput error when passing datetime strings #260
Conversation
The datetime `fromisoformat` method was introduced in Python 3.7, so can't be used here while retaining Python 3.6 compatibility. - Added python-dateutil requirement. - Use `dateutil.parse.isoparse` to detect datetime strings. - Update `mws.future_utils.params.clean_date` docstring. - Sort imports
|
Experienced this same issue while upgrading from |
|
Apologies for delay, haven't found the time for review outside work and home life. Not forgotten, will come to this when I can. My initial take would have been to avoid the new dependency on python-dateutil, but I can't think of a better way to identify datetime strings at this time. It's also something I hadn't considered, as we had focused on supporting I don't see too much issue here, as long as we provide a good range of supported python-dateutil versions to prevent locking up pip dependencies. Will review all this fully when I can, though. |
|
@samamorgan, to avoid the new dependency on It was added in Python 3.7, so we would still need a fallback on requires = [
"python-dateutil; python_version < '3.7.0'",
"requests",
]And having the fallback in the parsing code: try:
from datetime import datetime
fromisoformat = datetime.fromisoformat
except AttributeError:
from dateutil.parser import isoparse as fromisoformat |
|
@GriceTurrble While I typically like to avoid external dependencies as well, @adamantike I'd prefer simply accepting the dependency rather than adding this complexity. The biggest drawback to your proposed approach is we're now importing on every function call in Python >= 3.7, which adds overhead to the function, making each call take longer. Edit: Further supporting this change, there's even a recommendation from the
|
|
@adamantike Sorry, I didn't answer your initial question. It does fix the issue, but doesn't work with 3.6. See the initial commit: 3b47e27 Failing check from that commit: https://github.com/python-amazon-mws/python-amazon-mws/actions/runs/665822944 |
Yes, it won't work for 3.6 or older, but for Python 3.7 installations and onwards, the new dependency could be avoided if we go with this approach based on Python version.
There's no performance overhead. The code snippet I added for the imports is meant to be at the beginning of the file, not inside the function. |
|
I'd argue that if we're worried about adding a well-known dependency then it shouldn't be added for any version. I see no measurable advantage to the additional setup and import complexity. |
|
I would really like for a fix to this issue to be merged, as it's a pretty common scenario, and an unexpected breaking change when trying to upgrade from 0.8.10 to a newer patch version. @GriceTurrble mentioned that adding a new external dependency could potentially delay the decision on merging this PR, so my proposed alternative is there for the project maintainers to decide on which approach has a better alignment with their opinions.
By the way, is this fix also needed in the |
|
Finally got some time in to check on current PRs here. :) Recently merged in #263, which changes a bit of how the encoding works in
I did a quick test locally to ensure that the same output is found when cleaning either a string or From my perspective, that seems like the simpler fix. And as this is all targeting older 0.8 version (while new development contains on 1.0dev), I think this change is no longer necessary. Please let me know if I've missed anything. |
|
@samamorgan I apologize for potentially breaking your code: I handled a merge conflict from this side to bring your code up to date with the changes from #263 , and that seems to have busted the tests. I confirmed locally that simply removing the new check that uses I'm going to close this out. If you find those new changes are not working as you expect, please ping me again, and we can work through it to get a final resolution. Thank you. 👋 |
Fixes #259