-
Notifications
You must be signed in to change notification settings - Fork 37
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
refactor!: rewrite str$contains()
#982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, some minor comments
R/expr__string.R
Outdated
#' # The inline `(?i)` syntax example | ||
#' pl$DataFrame(s = c("AAA", "aAa", "aaa"))$with_columns( | ||
#' default_match = pl$col("s")$str$contains("AA"), | ||
#' insensitive_match = pl$col("s")$str$contains("(?i)AA") | ||
#' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to showcase this
src/rust/src/lazy/dsl.rs
Outdated
Some(true) => self.0.clone().str().contains_literal(pat.0.clone()), | ||
_ => self.0.clone().str().contains(pat.0.clone(), strict), | ||
}) | ||
pub fn str_contains(&self, pat: &RPolarsExpr, literal: Robj, strict: Robj) -> RResult<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't pat
be Robj
here? I think that would remove the need for .0.clone()
below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
The conversion to Expr on the Rust side was shown to be sufficient and conversion on the R side was found to be unnecessary.
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
No description provided.