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

Rename xml_find_one() to xml_find_first() and deprecate xml_find_one() #92

Merged
merged 1 commit into from May 27, 2016

Conversation

jimhester
Copy link
Member

@jimhester jimhester commented May 27, 2016

No description provided.

#' @usage NULL
#' @export
xml_find_one <- function(x, xpath, ns = xml_ns(x)) {
.Deprecated("xml_find_first")
Copy link
Member

@hadley hadley May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside of formally deprecating (i.e. instead of throwing a message) is that this will possible create new warnings for revdeps, so we'll need to do a release announcement at least two weeks prior to release. We probably don't need to do that otherwise because we're only adding new features.

Copy link
Member Author

@jimhester jimhester May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run a revdep_check() now and see how many packages generate warnings, if it is a lot we can just use a message() instead of .Deprecated().

Copy link
Member Author

@jimhester jimhester May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are only 6 packages that call xml_find_one() on CRAN now, counts are number of search results (files) which contain calls to xml_find_one().

library(gh)
res <- gh("GET /search/code", q = "user:cran xml_find_one")

library(purrr)
names <- map_chr(res$items, function(x) x$repository$name)
table(names)
#> names
#>   docxtractr googlesheets          oai    rversions          rvg 
#>            3            1            1            1           16 
#>      svglite         xml2 
#>            4            4

Copy link
Member Author

@jimhester jimhester May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the xml_attrs() change to output namespaces as well will likely break some packages, so we may need to do the release announcement regardless...

Copy link
Member

@hadley hadley May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH a message won't force anyone to notice the change. I can probably find time to fix svglite. Maybe we give people a month? That would make a CRAN release around May 27, which would line up nicely with useR.

@codecov-io
Copy link

codecov-io commented May 27, 2016

Current coverage is 59.33%

Merging #92 into master will decrease coverage by 0.13%

@@             master        #92   diff @@
==========================================
  Files            30         30          
  Lines          1345       1345          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            800        798     -2   
- Misses          545        547     +2   
  Partials          0          0          

Powered by Codecov. Last updated by c93169a...92a1fff

@jimhester jimhester merged commit 8a78f9b into r-lib:master May 27, 2016
3 checks passed
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.

None yet

3 participants