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

implement regex::Replacer for String, &String, Cow<'a, str>, &Cow<'a, str> #728

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

nooberfsh
Copy link
Contributor

It is inconvenient to get &str in some cases, for example:

extern crate regex; // 1.4.2
extern crate chrono; // 0.4.19

fn main() {
    let input = "today is time";
    let today = chrono::Local::now();
    let re = regex::Regex::new(r"time").unwrap();
    let s = re.replace(input, &*format!("{}", today)); // <-- I have to use &*format(...)
    println!("{}", s);
}

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Thanks! I think this LGTM, but I'd like to see two changes before merging it:

  1. Update the docs on the Replacer trait. Right now, it calls out the &str impl specifically, but now we've added more. I don't think we should list out every impl, but maybe say, "implemented for &str along with other variants of string types." Or something like that.
  2. Please also add the analogous trait impls in src/re_bytes.rs. It seems weird to offer these extra impls for only the &str API.

@nooberfsh
Copy link
Contributor Author

@BurntSushi updated.

@nooberfsh nooberfsh requested a review from BurntSushi January 12, 2021 09:42
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@BurntSushi BurntSushi merged commit 2bab987 into rust-lang:master Jan 12, 2021
This was referenced Mar 15, 2021
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.

2 participants