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

xml_child() function to make selection easier #94

Merged
merged 1 commit into from May 31, 2016

Conversation

Projects
None yet
3 participants
@jimhester
Member

jimhester commented May 27, 2016

With the current implementation you can do the following

x <- read_xml("<x><y/><z><y/></z></x>")

xml_child(x)
#> {xml_node}
#> <y>
xml_child(x, 2)
#> {xml_node}
#> <z>
#> [1] <y/>
xml_child(x, "y")
#> {xml_node}
#> <y>
xml_child(x, "z")
#> {xml_node}
#> <z>
#> [1] <y/>

Some questions before I add docs/tests

  • Is this a good function signature or were you thinking of something different?
  • Do we also want this same functionality for xml_sibling() and xml_parent()?
  • Do we want these functions to be generic with methods for xml_node and xml_nodeset?
  • Do we want to modify the existing xml_parent() to use this same signature or keep it as-is?
xml_children(x)[[search]]
} else if (is.character(search)) {
xml_find_first(x, xpath = paste0("./", search), ns = ns)
}

This comment has been minimized.

@hadley

hadley May 27, 2016

Member

Needs else that throws error

@hadley

This comment has been minimized.

Member

hadley commented May 27, 2016

  1. Yes, that looks right to me.
  2. I don't think so.
  3. I don't think it needs to be since xml_children() is already generic.
  4. Leave as is.
#'
#' @inheritParams xml_name
#' @param only_elements For \code{xml_length}, should it count all children,
#' or just children that are elements (the default)?
#' @param search For \code{xml_child}, either the child number to return (by
#' position), or the name of the child node to return.

This comment has been minimized.

@hadley

hadley May 27, 2016

Member

"If there are multiple child nodes with the same name, the first will be returned"

@codecov-io

This comment has been minimized.

codecov-io commented May 27, 2016

Current coverage is 58.89%

Merging #94 into master will decrease coverage by 0.69%

@@             master        #94   diff @@
==========================================
  Files            30         30          
  Lines          1356       1355     -1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            808        798    -10   
- Misses          548        557     +9   
  Partials          0          0          

Powered by Codecov. Last updated by d7b883d...db4538d

@jimhester

This comment has been minimized.

Member

jimhester commented May 31, 2016

@hadley anything else you think needs to change with this or should I merge it?

@hadley

This comment has been minimized.

Member

hadley commented May 31, 2016

LGTM

@jimhester jimhester merged commit d2c08a7 into r-lib:master May 31, 2016

3 checks passed

codecov/patch 100% of diff hit (target 59.59%)
Details
codecov/project 59.88% (+0.27%) compared to d7b883d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment