add support for formatted strings (fixes #130)

[freddyb]

I'm relatively new to rust¹ and this is my first Trait implementation. Maybe like this?

---

¹ I've done something like this in other languages before and I sorta know IRC. So, I hope this isn't too embarrassing :)

[aatxe]

Thanks for the PR, I'll go through and leave some comments. 😄

[freddyb]

(making some progress, found some errors and fixing locally)

[freddyb]

Thanks for the speedy review. I've got a bit further.

But I can't find a way to create the regex early on and just keep it around. Now I wonder where I'd want to create it to avoid recompilation :thinking:

error[E0015]: calls in constants are limited to struct and enum constructors
  --> src/proto/colors.rs:10:29
   |
10 | const STRIP_COLORS: Regex = Regex::new(r"\x1F|\x02|\x12|\x0F|\x16|\x03(?:\d{1,2}(?:,\d{1,2})?)?").unwrap();
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[aatxe]

There's a few ways forward that I see:

1. Just create the regex every time. I'm not sure how expensive it is to compile, but it's a pretty straightforward regex, so maybe not too bad. The documentation for regex suggests maybe a few microseconds for "small" regexps.
2. We can use the lazy_static crate to lazily initialize it. The downside is that this is another dependency.
3. Instead of a trait, we could have some struct like IrcFormatting that could keep the regex as internal state. This would be a rather different API though because you'd have to construct an instance of it in advance and use it as opposed to just write some_normal_string.strip_irc_formatting().
4. Hand-write the stripping procedure without regex. This is a bit harder to get right because of the way the colors work (I have a probably buggy implementation for parsing IRC formatting in alectro, you can find it split between here and here), but it doesn't require any extra dependencies.

I think I'm inclined to say 2 or 4 make the most sense. If this requires two dependencies (regex and lazy_static), I'll probably end up making it optional via a feature, but that's alright. I think some time soon we're supposed to be able to do compile-time compilation for regexps and then we might even be able to get rid of lazy_static at that point.

[freddyb]

I'll try 4, i.e., implementing this without regexes. Without having looked at your implementation, my naive approach would be to go through all characters and drop those for normal,underline,bold,reverse. I'm not sure how to exactly to look ahead to see how much more I'm supposed to drop, when I encounter the color character. Maybe I could peek() and then mark them for deletion in a following iteration.

I'm sure I'll figure something out, once I'm back at a computer.

[freddyb]

I've rewritten this into its own repo, to have something that's easier tested in its own, the tests pass, butbut I'm not happy with the solution at all :| Maybe you want to take a look anyway?

https://github.com/freddyb/leap

[aatxe]

Hi @freddyb, sorry for the delay. Your solution in leap looks great! My only suggestions are really stylistic: it's more idiomatic to use match in this context (with pattern guards for the extra conditions), and also to not use return. You might also find it helpful to run clippy (which I think should suggest both of those things and maybe others 😄). You might find it a bit easier to run clippy and make those changes in your little leap repo though because we already have some lints that need to be fixed in the irc crate itself and will add noise.

[freddyb]

Thanks for the pointer. I've rewritten this using match statements and I'm much happier with the readability. It still lives in https://github.com/freddyb/leap/blob/master/src/lib.rs but I'll update my pull request to use that function in the following days.

[aatxe]

Sounds good! Looking forward to it! 😄

[aatxe]

Oh, I forgot one other small thing! You can replace the test on line 30 (negated ||'s) with !['\x02', '\x1F', '\x16', '\x0F'].contains(cur) which is a bit clearer.

[freddyb]

sorry for taking so long @aatxe, but it works now. Can you review again?

[freddyb]

I've addressed your smaller comments. Currently trying to figure out how to create a test submodule and where to place it. But feel free to merge right now and file another issue for that, if you want.

[freddyb]

Also, I've squashed commits :)

[aatxe]

You should be able to put something like:

#[cfg(test)]
mod test {
...
}

at the end of colors.rs with all of the tests in it. You can see this pattern a lot throughout the irc crate, but here's a good example for Messages.

[freddyb]

now with tests :heavy_check_mark:

[aatxe]

Looks great! Thanks, @freddyb! 😄