8000 Remove rouge "values" key when building ObjectDict from xml by tomdottom · Pull Request #121 · python-amazon-mws/python-amazon-mws · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Jan 4, 2025. It is now read-only.

Remove rouge "values" key when building ObjectDict from xml #121

Closed
tomdottom wants to merge 1 commit into from
Closed

Remove rouge "values" key when building ObjectDict from xml #121

tomdottom wants to merge 1 commit into from

Conversation

@tomdottom
Copy link
@tomdottom tomdottom commented Jun 26, 2019

Before
Pasring produces unwanted entries of 'value': '\n '

>>> from mws.utils import XML2Dict
>>> sample_xml = """
...     <note>
...         <to>Tove</to>
...         <from>Jani</from>
...         <heading>Reminder</heading>
...         <body>Don't forget me this weekend!</body>
...     </note>
... """
>>> XML2Dict().fromstring(sample_xml)
{
    'note': {
        'value': '\n        ',
        'to': {'value': 'Tove'},
        'from': {'value': 'Jani'},
        'heading': {'value': 'Reminder'},
        'body': {'value': "Don't forget me this weekend!"}
    }
}

After

>>> from mws.utils import XML2Dict
>>> sample_xml = """
...     <note>
...         <to>Tove</to>
...         <from>Jani</from>
...         <heading>Reminder</heading>
...         <body>Don't forget me this weekend!</body>
...     </note>
... """
>>> XML2Dict().fromstring(sample_xml)
{
    'note': {
        'to': {'value': 'Tove'},
        'from': {'value': 'Jani'},
        'heading': {'value': 'Reminder'},
        'body': {'value': "Don't forget me this weekend!"}
    }
}

@codecov
Copy link
codecov bot commented Jun 26, 2019 •
edited
Loading

Codecov Report

Merging #121 into develop will decrease coverage by 30.44%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #121       +/-   ##
============================================
- Coverage    77.27%   46.83%   -30.45%     
============================================
  Files           18        4       -14     
  Lines          955      632      -323     
  Branches        92       64       -28     
============================================
- Hits           738      296      -442     
- Misses         212      324      +112     
- Partials         5       12        +7
Impacted Files Coverage Δ
mws/utils.py 72.81% <100%> (+9.85%) ⬆️
mws/mws.py 44.6% <0%> (-23.47%) ⬇️
mws/__init__.py 100% <0%> (ø) ⬆️
mws/apis/subscriptions.py
mws/apis/finances.py
mws/apis/reports.py
mws/decorators.py
mws/apis/orders.py
mws/apis/outbound_shipments.py
mws/apis/__init__.py
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49dc79d...0725d9d. Read the comment docs.

@tomdottom
Copy link
Author
tomdottom commented Jun 26, 2019

Addresses What are so many 'value': '\n ' for? in #69

@tomdottom tomdottom changed the base branch from master to develop June 27, 2019 16:15
@tomdottom tomdottom force-pushed the small-xml2dict-fix branch from 7cd54fd to 0725d9d Compare June 27, 2019 16:21
@Bobspadger
Copy link
Member
Bobspadger commented Jun 28, 2019

on the face of things I can't see any issues with this.

Would there be any use cases where that empty string may be required (I can't think of any)

@elcolumbio elcolumbio mentioned this pull request Jul 1, 2019
Merged
@elcolumbio
Copy link
Contributor
elcolumbio commented Jul 1, 2019

thank you for this. I referenced my pull request from 1 year ago where we agreed to use xmltodict.
Because our xmltodict is also handling cdata wrong.
The parsed data looks kinda different but it seems to work for everyone.
We should deci 8000 de if we merge something like #75 or if we dont. Or just merge and fix on the go since we have tested it and #75 has better test coverage than the main branch.

@tomdottom
Copy link
Author
tomdottom commented Jul 1, 2019

@elcolumbio using xml2dict seems like a reasonable course of action.

However, #75 appears to be a much larger piece of work as evidenced that it has been open for over a year.

Would there be any issue with merging this and creating a patch release to be able to get some of the benefits sooner? If anything, it would be one step closer to the behaviour of xml2dict.

@Bobspadger
Copy link
Member
Bobspadger commented Jul 1, 2019

@tomdottom just re-reading #69 quickly, and @GriceTurrble has pointed out some edge cases that may be affected by changing the current behaviour.

Could you write some tests to show that these edge cases would not be affected by your change?

If they are not, then its probably safe to merge and get some benefits on the develop branch.

@elcolumbio for your merge request, I would like to see @GriceTurrble sign that one off or @jameshiew as its has a lot more changes in there.

@elcolumbio
Copy link
Contributor
elcolumbio commented Jul 1, 2019

I still think we should just merge #75 to develop and use the next month for user feedback.

I had a brief look at your code. One tiny issue: In your CLI you used the new syntax: XML2Dict in your commit you didn't.
I wrote a test for #75 , this one failed here for the value None, because your if condition is assuming you have a string -> if None.strip() -> Error. Quite sure this if condition has to be modified.

@elcolumbio
Copy link
Contributor
elcolumbio commented Jul 1, 2019 •
edited
Loading

I am just getting started again, probably i make a lot of mistakes. We get binary strings from the amazon api and i think it's easier to solve those issue by writing the test input in binary strings. Like here, also what i referenced before:

python-amazon-mws/tests/test_wrappers.py

Lines 51 to 73 in 7e387b0

def test_DataWrapper_for_xml():
content = b'<ListInventorySupplyResponse \
xmlns="http://mws.amazonaws.com/FulfillmentInventory/2010-10-01/">\n \
<ListInventorySupplyResult>\n \
<MarketplaceId>A1PA6795UKMFR9</MarketplaceId>\n \
<InventorySupplyList/>\n </ListInventorySupplyResult>\n \
<ResponseMetadata>\n \
<RequestId>12bdfc18-8ccd-410d-ad33-31d5345d7b17</RequestId>\n \
</ResponseMetadata>\n</ListInventorySupplyResponse>\n'
encoding = 'ISO-8859-1'
response = FakeResponse(content)
y = DataWrapper(response, rootkey='ListInventorySupplyResult')
# here we test the encoding used by request.text function
# we don't aim for y.original.encoding == encoding
assert y.original.text == content.decode(encoding)
# pydict is not using the rootkey at the very moment
assert y.pydict == {'ListInventorySupplyResult': {'InventorySupplyList': None,
'MarketplaceId': 'A1PA6795UKMFR9'},
'ResponseMetadata': {'RequestId': '12bdfc18-8ccd-410d-ad33-31d5345d7b17'}}
# pytest can compare the dict representation
assert vars(y.parsed) == {'_DotDict__data': {'InventorySupplyList': None,
'MarketplaceId': 'A1PA6795UKMFR9'}}

@tomdottom
Copy link
Author
tomdottom commented Jul 1, 2019

Firstly @Bobspadger thanks for getting the Danger Mouse theme tune stuck in my head 🎵 ... He's fantastic ... 🎵

Moving on, are the edge cases from @GriceTurrble that you eluded to enumerated anywhere (issues, project tasks, etc ...)?

It would help to know which pathways, or upstream/downstream dependencies, of the current Xml2Dict
class you think are essential to test.

Cheers

@elcolumbio
Copy link
Contributor
elcolumbio commented Jul 1, 2019

It is possible to rewrite our own XML2Dict, or we use the python module xmltodict.
Even the process with the python module xmltodict is not trivial and opinionated.
The simplest solution for you looks somehow like this, but sometimes you will butcher data, but probably no less than with the main branch. Notice the data is even less nested and arguably nice than in after your parsing.

import xmltodict

xmltodict.parse(example_response, dict_constructor=dict,
                process_namespaces=False,
                force_cdata=False)

@tomdottom tomdottom closed this Aug 8, 2019
@probeyang
Copy link
probeyang commented Nov 18, 2019

Addresses What are so many 'value': '\n ' for? in #69
i used the code:
Remove rouge "values" key when building ObjectDict from xml #0725d9d

but, lot of "values" was there too.
if node.text.strip(): node_tree.value = node.text.strip()

i used the orders modules,and "get_order" interface.
how to do it? thanks!

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tomdottom @Bobspadger @elcolumbio @probeyang
0