Skip to content

Conversation

@slobo
Copy link
Contributor

@slobo slobo commented Dec 26, 2019

This makes t/10_responses.t pass

@slobo
Copy link
Contributor Author

slobo commented Dec 26, 2019

So, in this first iteration, we learn that SuppressEmpty => undef behaviour is the only difference that test suite can uncover for 10_responses.t. But, while doing this in perl code is probably fast enough for smaller results at least, it feels rather dirty, so I'll try to petition XML::Hash::XS to add equivalent behaviour.

@slobo slobo changed the title Emulate XML::Simple SuppressEmpty option Use XML::Hash::XS Dec 28, 2019
@slobo
Copy link
Contributor Author

slobo commented Dec 28, 2019

@pplu, with this change, test suite passes on my machine (minus some failure on 01_load.t, but same happens on the base branch, and I hope is unrelated). Let me know if there is anything else we need to do. Thanks!

@castaway
Copy link
Collaborator

Looks like we should look at merging this for 0.44 (before 0.45), see #372

@byterock
Copy link
Collaborator

Hmm might clean up the issue I had with a few of the S3 actions where the parsing the XML beheaded it and I had to add in a little extra logic to handle the missing root key.

Would have to look at my notes to see which one.

@castaway
Copy link
Collaborator

There are a few responses that don't contain the main XML key (there's code in RestXMLResponse to handle it) - would be handy to get rid of XML::Simple regardless

@slobo
Copy link
Contributor Author

slobo commented Feb 28, 2020

@pplu , this PR just merges into your xml2hash branch, so probably safe to accept and continue work in there?

@slobo
Copy link
Contributor Author

slobo commented Feb 28, 2020

Or, should we maybe change the target to be the next release branch to see how it behaves?

@castaway
Copy link
Collaborator

castaway commented Mar 2, 2020

working into xml2hash seems sane to me, I hadnt seen it was against that one

@mschout
Copy link

mschout commented Mar 6, 2020

Would love to see this stop-gap make it into a release.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants