liquid-rust icon indicating copy to clipboard operation
liquid-rust copied to clipboard

Semantic errors include line numbers where possible

Open epage opened this issue 6 years ago • 3 comments

Expanding on #232, we can capture the line associated with a block / filter / etc and report that during the render phase.

epage avatar Nov 30 '18 23:11 epage

@Goncalerta

This is part of a broader problem because not only would it be good to show line numbers on FilterChain, it would be also nice to have them on other places (such as the rendering phase) to report other errors (for example, a non-existing variable). The way I see this being done is by passing the line number information when creating structures such as FilterChain, Value, etc... (maybe wrapped in some kind of 'Context' or 'Position' structure) that they would then give to liquid error when something goes wrong.

Before this, though, liquid error should probably have some improvements, so it explicitly stores the line numbers and such contexts instead of plain copying the string from a Pest error.

From https://github.com/cobalt-org/liquid-rust/issues/303

epage avatar Dec 13 '18 18:12 epage

To be clear, line numbers during render is more of https://github.com/cobalt-org/liquid-rust/issues/247 so I split this conversation off here so we can capture our thoughts. #303 is more focused on the fact that for assign, the parser logic is swallowing the list of available filters. I happened to also call out the way that the filter chain is inferior to the assign error.

I agree that we need to pass location information to tags/blocks.

In all of this, we also need to keep https://github.com/cobalt-org/liquid-rust/issues/248 in mind.

What value do you see in liquid error tracking line number vs just rendering the message with file/line? I've not really thought much on this.

epage avatar Dec 13 '18 18:12 epage

What value do you see in liquid error tracking line number vs just rendering the message with file/line? I've not really thought much on this.

For instance, we would be able to do calculations with the value (which might be useful for #248). It would also allow to create and format our own Liquid error messages, instead of copying Pest messages, so as to have more control (in particular, the part that says "unexpected ____" or "expected ____". Right now I don't see any practical need for that, but it could be helpful in the future). The Debug format of our error message right now is strange and hard to read. Instead of having every information on the error, it has everything mixed into a string, so we get confusing things with a lot of \n and some text that would only be relevant in the Display mode of the error. Pest errors would only exist during parsing, so we would need to generate our error messages anyway for other errors. I think it would be easier to keep consistency if both parsing errors and other errors would be treated equally (so both would be generated by our Error message).

Goncalerta avatar Dec 13 '18 19:12 Goncalerta