Generate better errors for top level nodes #42
Conversation
rinja_derive/src/heritage.rs
Outdated
| Node::Extends(_) | Node::Macro(_) | Node::Import(_) if !top => { | ||
| return Err(CompileError::no_file_info( | ||
| "extends, macro or import blocks not allowed below top level", | ||
| Node::Extends(e) if !top => { |
There was a problem hiding this comment.
I'm a bit sad about needing to split this in 3 parts (because the Node isn't the same, so rust isn't happy). But we got a small improvement for this: the error message is now specific to the node type.
There was a problem hiding this comment.
I feel the same. A name() on node, optionally with a top_level_only(), might reduce the specialization.
The bigger problem I find using Rinja (or Askama) is the lack of context in the error messages for templates. It's nice to know what specific keyword caused the problem but it would be even better if it showed a template line number.
There was a problem hiding this comment.
It's exactly what this PR adds. Take a look at the error output here. Almost all errors now have context and everything minus a few exceptions. :)
There was a problem hiding this comment.
Very helpful, thank you!
Anything else that needs doing for the merge?
There was a problem hiding this comment.
Getting a review from @Kijewski. For now we're two maintainers and we follow the rule to wait for at least one other maintainer's review before merging. So just a bit of patience now. 😉
| "#, ext = "txt")] | ||
| struct MyTemplate3; | ||
|
|
||
| #[derive(Template)] |
There was a problem hiding this comment.
This might not be the best location for this test as it checks multiple extends rather than blocks not at the top level
There was a problem hiding this comment.
Fair enough, moving it to its own file.
rinja_derive/src/heritage.rs
Outdated
| Node::Extends(_) | Node::Macro(_) | Node::Import(_) if !top => { | ||
| return Err(CompileError::no_file_info( | ||
| "extends, macro or import blocks not allowed below top level", | ||
| Node::Extends(e) if !top => { |
There was a problem hiding this comment.
Very helpful, thank you!
Anything else that needs doing for the merge?
GuillaumeGomez
force-pushed
the
top-level-nodes-errors
branch
from
a7b386b to
27e12f0
Compare
Kijewski
force-pushed
the
top-level-nodes-errors
branch
from
3daa777 to
d4b363b
Compare
|
Very useful change! I de-duplicated the top-level check, and getting the FileInfo of a Node a bit. |
|
I can't approve it says github but I like the changes a lot, thanks to both of you! |
No description provided.