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_all mimics str_replace #362

Closed
shosaco opened this issue Sep 30, 2019 · 2 comments · Fixed by #392
Closed

str_replace_all mimics str_replace #362

shosaco opened this issue Sep 30, 2019 · 2 comments · Fixed by #392
Labels
bug func trans 🌍 help wanted ❤️

Comments

@shosaco
Copy link
Contributor

shosaco commented Sep 30, 2019

The current implementation of str_replace_all which was introduced in PR #131 for postgres backends has no global parameter, thus replacing only the first occurence of any match in a string:

File R/backend-postgres.R, line 58ff:

str_replace_all = function(string, pattern, replacement){
        sql_expr(regexp_replace(!!string, !!pattern, !!replacement))
      }

To my mind, the correct implementation would be

str_replace_all = function(string, pattern, replacement){
        sql_expr(regexp_replace(!!string, !!pattern, !!replacement, 'g'))
    }

Also, the fix of str_detect has disappeared from master and it is not possible to grep for regular expressions on Postgres databases at the moment: Line 55 in beforementioned file quotes

      str_detect  = function(string, pattern) {
        sql_expr(strpos(!!string, !!pattern) > 0L)
      }

instead of the line that was implemented Pull request #131:

str_detect = sql_infix("~"),

Thanks for the great effort and package!
sandro

@hadley hadley added bug func trans 🌍 help wanted ❤️ labels Dec 13, 2019
@hadley
Copy link
Member

hadley commented Dec 13, 2019

Would you be interested in doing a PR?

@shosaco
Copy link
Contributor Author

shosaco commented Dec 17, 2019

Would you be interested in doing a PR?

Created PR #392

best regards,
sandro

hadley pushed a commit that referenced this issue Apr 15, 2020
Corrected `str_detect()` so it uses regular expressions, introduced `str_replace()` and corrected `str_replace_all()`.

Fixes #362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug func trans 🌍 help wanted ❤️
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants