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

improvements #1

Open
BurntSushi opened this Issue Mar 20, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@BurntSushi
Copy link

BurntSushi commented Mar 20, 2015

So I love the experiment, and I just wanted to suggest a couple things that might increase its chances of succeeding.

Firstly, you might want to consider using just the standard regex crate without the macro. This is because regex! is a compiler plugin, which means it will not work on the beta/stable channels of Rust 1.0.

Secondly, to increase the ergonomics of indexing, I'd recommend that you change your ReIdx::new constructor to take a &str, which would then call the Regex::new constructor for the user. e.g.,

impl ReIdx {
    fn new(re: &str) -> ReIdx {
        ReIdx(Regex::new(re).unwrap())
    }
}

Or even better (I should probably change Regex::new to use a similar interface):

    fn new<'a, S: IntoCow<'a, str>>(re: S) -> ReIdx {
        ReIdx(Regex::new(&re.into_cow())
    }

Then you could index like "hello"[ReIdx::new(r"el")]. You might go even further and define a short macro, rei! (or something): "hello"[rei!(r"el")].

@ArtemGr

This comment has been minimized.

Copy link

ArtemGr commented Mar 20, 2015

Erm. Compiling the regular expression every time isn't cool, so I hope there will be some ReIdx::from_regex left around or something.

On a side note, a no-brainer version would've looked "hello"[regex!(r"el")], but that's probably not possible to implement outside of the regex crate. Both "hello"[ReIdx::new(r"el")] and "hello"[rei!(r"el")] fall short of being as fluent as the Ruby version, IMHO. J-:

@BurntSushi

This comment has been minimized.

Copy link
Author

BurntSushi commented Mar 20, 2015

regex! will not work on the beta/stable channels of Rust 1.0.

Alternate constructors seem very appropriate.

@kstep

This comment has been minimized.

Copy link
Owner

kstep commented Mar 20, 2015

@BurntSushi do you know when regex! macro will be stabilized? It feels like a loss to be unable to compile regexps in compile time.

@BurntSushi

This comment has been minimized.

Copy link
Author

BurntSushi commented Mar 21, 2015

@kstep Of course it's a loss! :-) Stabilization on a schedule isn't free---we have to make sacrifices somewhere. The regex! macro is a compiler plugin, and the API for compiler plugins currently depends on compiler internals, and there's just no reasonable way to stabilize that API before 1.0.

I don't know the time table. Compiler plugins are really nice to have, but it will take some work to turn them into a stable feature.

@kstep

This comment has been minimized.

Copy link
Owner

kstep commented Mar 21, 2015

Ok, check out version 0.2.0, please. @BurntSushi, it wasn't quite possible to implement new<T: IntoCow>(s: T) method the way you wanted without possible panic due to the fact Regex::new() returns Result. I added a macro ri instead (I decided to make the name shorter), so one can index in the following way:

let a = &"abbcccdddd"[ri!["bc+"]];

This is as far eye-candy as possible without bothering regex crate.

@ArtemGr

This comment has been minimized.

Copy link

ArtemGr commented Mar 21, 2015

Looks like you don't need #![plugin(regex_macros)] on the second line. Nor regex_macros = "*" in Cargo.toml.

@kstep

This comment has been minimized.

Copy link
Owner

kstep commented Mar 21, 2015

After a little thought (bf732a39) I decided to add convenience impl Index<Result<ReIdx, regex::Error>>, so you don't have to unwrap regex result to index. A downside of this approach is you will just get empty string for Err(_) variant.

@kstep

This comment has been minimized.

Copy link
Owner

kstep commented Mar 21, 2015

@ArtemGr, you are right, now that I don't use regex! in tests any more I don't need to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.