-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
parser: tell user proper keyword to end node #118
Conversation
1970e2a
to
39e9a1d
Compare
rinja_parser/src/node.rs
Outdated
Ok((i, actual)) | ||
} else if actual == "end" { | ||
Err(nom::Err::Failure(ErrorContext::new( | ||
format!("`{node}` nodes are terminated with `{expected}`"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reword it a bit to make it more obvious that the "ender" is missing:
format!("`{node}` nodes are terminated with `{expected}`"), | |
format!("missing `{expected}` to terminate `{node}` node"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought my wording expresses better that end
is not a keyword. Maybe the combined message would be best?
"\
missing `{expected}` to terminate `{node}` node
`{node}` nodes are terminated with `{expected}`, not `end`"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not entirely sold on your message improvement. Also, thinking about this some more, I think the else if actual == "end"
block should be merged with the else
.
Message suggestion v3:
expected `{expected}` to terminate `{node}` node, found `{actual}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
else if actual == "end"
block should be merged with the else.
Not possible. fail()
returns an recoverable error, but for actual == "end"
we return an unrecoverable failure. (The name fail()
might not have been the best choice.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. We tell that it looks to be looking further because it doesn't match the need here. Hum... Did you encounter this problem yourself? (using end
instead of the right keyword)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I searched what usage questions users were having about askama, and came across this thread: https://users.rust-lang.org/t/how-to-solve-error-about-askama/59431
I added a commit that shows the error messages before this PR: 0628afa. I think it's much better now. :) |
I like it, thanks for going along! Very good idea to look at issues askama users encountered. :) |
Thanks! :) Yes, I can always try to imagine what problems other people might have, but my knowledge of the project is much different from other users. |
No description provided.