Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Semantic errors include line numbers where possible #247

Open
epage opened this issue Nov 30, 2018 · 3 comments
Open

Semantic errors include line numbers where possible #247

epage opened this issue Nov 30, 2018 · 3 comments
Labels
enhancement Improve the expected

Comments

@epage
Copy link
Member

epage commented Nov 30, 2018

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

@epage epage added the enhancement Improve the expected label Nov 30, 2018
@epage
Copy link
Member Author

epage commented Dec 13, 2018

@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 #303

@epage
Copy link
Member Author

epage commented Dec 13, 2018

To be clear, line numbers during render is more of #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 #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.

@Goncalerta
Copy link
Collaborator

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).

epage added a commit to epage/liquid-rust that referenced this issue Dec 22, 2018
The goal of this is so that runtime includes of partials can set this
and then any error (usually the `trace`) that happens within that partial can be prefixed with
the name.  Eventually we also want these to include line numbers (see cobalt-org#247).

In the end, this will make the errors look more like what you'd expect
from a compiler error.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

2 participants