Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

as_list.xml_node drops element names if any xml attributes exist #115

Closed
peterfoley opened this issue Jul 27, 2016 · 4 comments
Closed

as_list.xml_node drops element names if any xml attributes exist #115

peterfoley opened this issue Jul 27, 2016 · 4 comments

Comments

@peterfoley
Copy link
Contributor

@peterfoley peterfoley commented Jul 27, 2016

Reproducible example:

txt <- "<?xml version=\"1.0\" encoding=\"UTF-8\"?>
<level_0 xmlns=\"http://whatever.com\">
  <level_1a> 
    <item>item1a</item>
  </level_1a>
  <level_1b>
    <item>item1b</item>
  </level_1b>
</level_0>
"

packageVersion("xml2")
library(xml2)
parsed <- as_list(read_xml(gsub("\\n\\s*","",txt)))
names(parsed)

The issue appears to be line 58 in:
attributes(out) <- as.list(attr)
which overwrites the names that were previously stored by line 52.

@leeper
Copy link

@leeper leeper commented Aug 8, 2016

I am encountering the same issue with code that worked prior to xml2 v1.0.0 but now fails. I know this particular line of code was not changed in in the v1.0.0 release, but it appears to be interacting with other changes (probably in xml_attrs() methods).

Two difference changes could resolve this (for my use case):

  1. Removing lines: https://github.com/hadley/xml2/blob/master/R/as_list.R#L58-L59
  2. Replacing those lines with:
 if (length(attr) > 0)
    attributes(out) <- c(attributes(out), as.list(attr))

The latter is probably better. Should I submit a PR?

@peterfoley
Copy link
Contributor Author

@peterfoley peterfoley commented Aug 8, 2016

There's already a PR basically done for this issue, just needs to be merged. The original issue was motivated by xml2 v1.0.0 breaking cloudyr/aws.ec2, so hopefully it'll also fix what was broken for cloudyr/aws.s3.

@leeper
Copy link

@leeper leeper commented Aug 8, 2016

Yes, it should. Thanks for that!

@jimhester
Copy link
Member

@jimhester jimhester commented Aug 8, 2016

Sorry for the delay in merging, the current master should now be fixed.

@jimhester jimhester closed this Aug 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants