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

str_replace should ignore NA when using function as replacement #164

Merged
merged 11 commits into from Dec 28, 2017

Conversation

yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented May 12, 2017

I like str_replace() with function replacement. It's a very cool feature.

One thing, currently, the code bellow returns a vector contains NA:

text <- c("A1", "B1", "CC")
pattern <- "[A-Z][0-9]"

stringr::str_replace(text, pattern, tolower)
#> [1] "a1" "b1" NA

But, I feel this is a bit counter-intuitive and not consistent with the results of similar functions:

stringr::str_replace_all(text, pattern, tolower)
#> [1] "a1" "b1" "CC"

stringr::str_replace(text, pattern, "x")
#> [1] "x"  "x"  "CC"

This PR will

  1. add omit_na option to str_sub(), which stri_sub<-() got in response to gagolews/stringi#199 and I'm trying to extend in gagolews/stringi#268.
  2. use it in str_replace() to ignore NA and keep the original.

Note that, test will pass only after gagolews/stringi#268 will be merged.

@yutannihilation yutannihilation changed the title str_replace should ignore NA when using function as replatement str_replace should ignore NA when using function as replacement May 12, 2017
@hadley
Copy link
Member

@hadley hadley commented Nov 14, 2017

Is this now in CRAN stringi?

@gagolews
Copy link
Contributor

@gagolews gagolews commented Nov 14, 2017

Should be anytime soon, the new release of stringi got stuck in CRAN/incoming since Friday

@hadley
Copy link
Member

@hadley hadley commented Nov 14, 2017

Great - @yutannihilation can you please include the stringi version requirement in the DESCRIPTION?

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 14, 2017

Thanks. I updated the version requirement to 1.1.6.

https://github.com/gagolews/stringi/releases/tag/v1.1.6.

R/sub.r Outdated
@@ -15,6 +15,8 @@
#' matrix to `start`.
#'
#' Negative values count backwards from the last character.
#' @param omit_na single logical value; if \code{TRUE}, missing values in any of the arguments
Copy link
Member

@hadley hadley Nov 14, 2017

Choose a reason for hiding this comment

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

Can you please switch to markdown formatting?

R/sub.r Outdated
@@ -15,6 +15,8 @@
#' matrix to `start`.
#'
#' Negative values count backwards from the last character.
#' @param omit_na single logical value; if \code{TRUE}, missing values in any of the arguments
#' provided will result in an unchanged input; replacement function only
Copy link
Member

@hadley hadley Nov 14, 2017

Choose a reason for hiding this comment

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

Missing indent here

Copy link
Member

@hadley hadley Nov 14, 2017

Choose a reason for hiding this comment

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

I think you can remove the bit about replacement function only. It would be useful to have a bit more motivation about when this is useful

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 14, 2017

Should I also add stringi to Remotes? Or can I just wait the release of 1.1.6?

@hadley
Copy link
Member

@hadley hadley commented Nov 15, 2017

Add to remotes just so we can test it, and I'll remove before releasing stringr.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 15, 2017

Thanks, I've added Remotes. Travis passes but AppVeyor fails to install stringi by this error. Needs to investigate later.

/bin/sh: c:/Rtools/mingw_32/bin/g++: No such file or directory
(https://ci.appveyor.com/project/tidyverse/stringr/build/1.0.22#L166)

@hadley
Copy link
Member

@hadley hadley commented Nov 15, 2017

Looks good. Can you please add a bullet to NEWS? It should briefly describe the change (starting with name of the function), and crediting yourself with (@yourname, #issuenumber).

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 15, 2017

Thanks! I've done. I think this is ready to merge now.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 18, 2017

Congratulations for the release of stringi 1.1.6! 🎉

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 23, 2017

@hadley Hi, do I need to make further improvements on this PR? (Just curious, I know it's a bit too late since the next version is already submitted to CRAN)

@hadley
Copy link
Member

@hadley hadley commented Nov 28, 2017

Unless it gets rejected by CRAN, it'll need to go in the next version. Sorry for forgetting about it 😢

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 28, 2017

No problem. Sorry for taking some time to get this PR ready. I will try to do better next time 😉

@hadley
Copy link
Member

@hadley hadley commented Dec 28, 2017

Luckily for you, stringr did get rejected, so I can merge this 😄

@hadley hadley merged commit d6ef15d into tidyverse:master Dec 28, 2017
4 checks passed
@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Dec 28, 2017

Wow, I appreciate the luck. Thanks!

@yutannihilation yutannihilation deleted the omit-na branch Dec 28, 2017
jdblischak added a commit to workflowr/workflowr that referenced this issue Oct 17, 2019
error.

I remember seeing this error at the BSD workshop back in April 2019,
but had no idea what was causing it. I happened to reproduce it
when in the RStudio Server hosted in the Docker image rocker/verse
(I wanted to test the httpuv error with the Viewer pane). I got
the same error message as some of the students:

wflow_start("test")
Error: evaluation nested too deeply: infinite recursion /
options(expressions=)?
Error during wrapup: evaluation nested too deeply: infinite recursion /
options(expressions=)?

It happened no matter the directory, or using the RStudio project
template.

The error was caused by the recursive function
obtain_existing_path(). It didn't have a stop criteria if it was
passed nonsense like NA, so it went on recursively until it errored.

The NA came from absolute() -> toupper_win_drive(). stringr 1.2.0
introduced the ability to use a function to perform the replacement.
This is how toupper_win_drive() works, it uses toupper. The problem
is that if there is no match, instead of returning the original
string, it returns NA.

This was fixed in stringr 1.3.0 in PR #164. The NEWS bullet is
quite subtle for how much it changes the behavior.

tidyverse/stringr#164
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