Text formatting with embedded markup for terminal output

I'm learning Rust. I wrote the following library to format text and output it to terminal, so that the formatting markup is embedded with the string itself (looks similar to HTML4).

I feel the code is pretty unidiomatic and I'd really appreciate hints on how to make it better, more readable and easier to extend.

The parser is ad-hoc and this is intentional - I don't want to depend on external libraries, as they tend to break often now.

Usage example can be found here.

Some specific points:

• I don't like how some Tokens contain Options inside (e.g. line 42). This means that in case user wants to use second interface of library - passing slice of tokens for rendering - they must specify all Options as containing Some(_), otherwise rendering will panic.

• I don't like the error handling - rendering will panic in a lot of places if fed with wrong stream of tokens. I believe this is the result of 1, but nevertheless.

• Propagating the Result, like at line 287 - there must be proper idiomatic way to do this, right?

• Iteration - lines 63 and 237 - I had to do the work in nested block since otherwise the borrow checker complains about two mutable references to iter. I feel like there's a better way nevertheless.

• Matching on &Something - line 245 - I had to do this because of borrow checker - it didn't allow to move the value out of immutable slice via iterator. I know matching on Something is preferred, but couldn't figure out the way to do it here.

• A slightly minor concern is duplication of attributes - I have the same ones in Token as term has in term::attr::*. Looks like another result of point 1.

Second half of the listing is tests and are provided for reference.

```
extern crate term;

use State::{Beginning, Tag, Inside, InsideColor, InsideBool};
use Token::{Fg, Bg, Bold, Dim, Italic, Underline, Blink, Standout,
Reverse, Secure,
Reset,
Literal};
use term::{Terminal, WriterWrapper};
pub use term

You have a lot of code and a lot of questions. I'll answer them as I have suggestions (and time).

General thoughts

parse is 186 lines long! I found it really difficult to navigate and understand it, much less make changes. I'd suggest trying to break out smaller methods.

Point 1

Why do these tokens have an Option in the first place? I only see one instance where you set it to None, but all your tests set it to Some and you always unwrap it in render. This seems like you always expect it to be present, so I don't understand why you chose an Option.

Edit 1: I think I get it now. You are in the process of reading through your string, so you don't know what the value will be yet. I'd suggest leveraging the type system to help you out here, which might mean more typing (both for the compiler and your hands). As a high-level example, you could have

enum PartialToken {
    Italic,
    Underline,
    // .. snip ..
}

When you get to the value, you can take the partial token and expand it out to the real token:

match partial_token {
    PartialToken::Italic => Token::Italic(value),
    // .. snip ..
}

You might be able to avoid creating a type if you can keep enough state in local vars to construct the whole token in one shot, but the parse method is too hairy for me to experiment with.

Point 2

I agree that the failure cases of render are all spread out. Here's my current suggested rewrite:

// Comment #1
pub fn render(term: &mut FullTerminal, tokens: &[Token]) -> std::io::IoResult {
    for t in tokens.iter() {
        let r = match *t {
            // Comment #2
            Literal(ref string)    => write!(term, "{}", string).map(|_| true),
            // Comment #3
            Fg(Some(color))        => term.fg(color),
            Bg(Some(color))        => term.bg(color),
            Bold                   => term.attr(attr::Bold),
            Dim                    => term.attr(attr::Dim),
            Italic(Some(value))    => term.attr(attr::Italic(value)),
            Underline(Some(value)) => term.attr(attr::Underline(value)),
            Blink                  => term.attr(attr::Blink),
            Standout(Some(value))  => term.attr(attr::Standout(value)),
            Reverse                => term.attr(attr::Reverse),
            Secure                 => term.attr(attr::Secure),
            Reset                  => term.reset().map(|_| true),
            // Comment #4
            Fg(None)               |
            Bg(None)               |
            Italic(None)           |
            Underline(None)        |
            Standout(None)         => panic!("Expected value missing"),
        };

        try!(r);
    }

    term.reset().map(|_| true)
}

// Comment #5
pub fn render_str(term: &mut FullTerminal, s: &str) -> Result {
    parse(s).and_then(|tokens| {
        render(term, tokens.as_slice()).map(|_| ()).map_err(|x| x.to_string())
    })
}

• Instead of letting render fail on it's own, let's just propagate the error up to someone who can decide how to deal with it.

• Here we use map to convert the IoResult to an IoResult. This allows the types to match, without adding too much confusion. term uses true to mean "it worked" or "it's supported", so that seems a close match.

• We match on the Some case explicitly to avoid calling it maybe_color and having to unwrap it.

• We deal with all the None cases at once. Here, I'm lazy and so I I panicked, but it would probably make more sense to not return an IoResult but some custom error type that can handle both cases equally well.

• Again another reason to have a custom error type, I convert from IoError to String and chain the two Results with and_then.

Point 3

You are looking for the try! macro:

pub fn render_str(term: &mut FullTerminal, s: &str) -> Result {
    let tokens = try!(parse(s));
    Ok(render(term, tokens.as_slice()))
}

Or maybe map:

pub fn render_str(term: &mut FullTerminal, s: &str) -> Result {
    parse(s).map(|tokens| render(term, tokens.as_slice()))
}

Point 4

I'm not sure why you can't use a regular for-loop:

fn parse(s: &str) -> Result, String> {
    // .. snip ..
    // Comment #1
    for i in s.chars() {
        match state {
            Beginning => // .. snip ..
        }
    }

    // Comment #2
    match state {
        Tag => // .. snip ..
    }

    Ok(tokens)
}

• Simply iterate over each character, no need for a peekableiterator, just one-by-one.

• At the end, after iterator has finished, do all the stuff that was in the None clause.

Point 5

I wouldn't say that matching on Foo is preferred to &Foo, they are each useful at different times. Perhaps the former is less cluttered than the latter? That being said, I was able to dereference the token once and then match on it like:

for t in tokens.iter() {
    match *t {
        Literal(ref string) => // ...

Point 6

The best way I know to fix

StackExchange Code Review Q#74029, answer score: 3