Fix error display if pos is after \n

[flying-sheep]

I had to add an API for moving a position back.

An alternative would have been to modify skip_while to take a slice of std::str::pattern::Patterns instead of &[&str] and add a skip_back_while function that does the same but backwards. Then we could have skipped back until we hit a non-newline character.

Should I do that instead?

Fixes #348

[dragostis]

Thanks for the PR! Maybe some of this logic should be behind Span? I.e. a normalization method that takes care of cases like this.

[flying-sheep]

Maybe some of this logic should be behind Span?

Maybe! Position already has the line_of function, so the primitives already do things that are specific to \n delimited input.

[flying-sheep]

Hmm, I think the normalization only makes sense for display purposes, so I left it in the error format code

[dragostis]

Taking a better look at this, maybe a generally better solution would be to have Position::line_of be aware of boundaries. In other words, if the Position is right after \n, it should choose the line after, while Position::left_line_col should return the previous one. Not so sure about the way the API should look here, but I think that this is a better solution.

[flying-sheep]

Hmm, but wouldn’t that mean that the next line is included in the error display?

Wouldn’t the Span(0,8) look like this?

 --> 1:1
  |
1 | abcdef
2 | 
  | ^
  |
  = error: big one

The honestly best thing I could imagine is this:

 --> 1:1
  |
1 | abcdef␊
  | ^-----^
  |
  = error: big one

with the “␊” in another color

[dragostis]

That's quite a neat solution! Here's an idea for the API: Span::lines will return Iterator<Item = String> with your LF fix. This will then be used by Error.

[flying-sheep]

OK. So implementing something like #349 in my own crate, I found myself missing public access to the Position::skip_back() I defined here.

Specifically I wanted to parse a slice of a Pair/Span to an u8. More specifically, a grammar I use has an optional % sign. I’d like to do this:

fn parse_scale(pair: Pair<Rule>) -> Result<u8, Error> {
    let input = if pair.as_str().chars().rev().next() == Some('%') {
        pair.as_span().start_pos().span(pair.as_span().end_pos().skip_back(1))
    } else { pair };
    // Pair.parse is supplied via #349, link above
    Ok(input.parse().map_err(|e| to_parse_error(input, e)))
}

…or something more elegant. How should I go about it? impl Index<RangeTo> for Span and friends?

[dragostis]

Implementing Index* for Span is probably not a great idea since the underlying &str doesn't implement them either. Span can be converted to a &str which can then be used in str::chars() to get an iterator over the characters. However, this break the lifetime. Maybe a proper implementation would be to have Span::utf8_slice(range: Range<usize>)?

[flying-sheep]

You are of course right. And this is probably not the right issue to discuss this. I only mentioned it because I introduced skip_back here, which could be used if it was public:

let new_span = span.start_pos().span(span.end_pos().skip_back(1))

[flying-sheep]

OK, LF proposal is done! please tell me where I should add comments.

[flying-sheep]

the errors are not mine, they’re caused by clippy on rust 1.31!

I also think that this has grown a bit out of proportion for starting as a bugfix, so maybe we should merge this once it’s qualitatively acceptable (if it isn’t already) and then do the cosmetics (e.g. coloring the error display) elsewhere.

[dragostis]

Thanks for bearing with me. This PR outgrew its purpose and I sure hope I'm dragging this too much. I'm fine with merging this as soon as last few issues are solved, but longer term, Span should get more functionality like the slicing you mentioned.

[flying-sheep]

Don’t worry, it’s fine, I’m happy to contribute

[CAD97]

It looks like you'll need to run a new cargo fmt now that we're checking that. If you haven't rebased to include the .rustfmt.toml changes yet, that'll change the formatting you get out of it.

[flying-sheep]

sure. please make sure to squash-merge this, no need to have the whole mess in the git history.

[dragostis]

Sorry for the late review. LGTM!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push

[flying-sheep]

woo! 🎉