Skip to content
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

Implement error propagation expression: ? #590

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Implement error propagation expression: ? #590

merged 2 commits into from
Jan 28, 2022

Conversation

Kijewski
Copy link
Collaborator

@Kijewski Kijewski commented Jan 4, 2022

This PR consists of three two a changes:

  • Right now almost every expression needs to be parsed twice: expr_any() first parses the left-hand side of a range expression, and if no .. or ..= was found the left-hand expression is parsed again, this time as the result of the function. This diff removes the second parsing step by first looking for .. (opt rhs), then for lhs .. (opt rhs).
  • Instead of having Expr::VarCall, Expr::PathCall and Expr::MethodCall, this diff unifies the handling of calls by removed the former three variants, and introducing Expr::Call. This makes parsing postfix operators easier.
  • This change allows using the operator ? in askama expressions. It works like the same operator in Rust: if a Result is Ok, it is unwrapped. If it is an error, then the render() method fails with this error value.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't done an in-depth review of the final commit yet, but here's some feedback on the prerequisite refactoring steps.

askama_shared/src/parser.rs Show resolved Hide resolved
askama_shared/src/error.rs Outdated Show resolved Hide resolved
@djc
Copy link
Collaborator

djc commented Jan 23, 2022

Having thought about this some more, I'm not sure I'm comfortable with the addition of the try operator. It seems like a bunch of complexity with a goal that I'm not sure meshes well with the design of Askama -- in my opinion, templates should generally be written not to yield errors. Did you have some concrete use cases where you wanted this behavior? I don't think fully matching the Rust grammar should be a goal unto itself.

(The changes to introduce a unified Call AST type seems nice, though.)

@Kijewski Kijewski mentioned this pull request Jan 25, 2022
This change allows using the operator `?` in askama expressions. It
works like the same operator in Rust: if a `Result` is `Ok`, it is
unwrapped. If it is an error, then the `render()` method fails with this
error value.
@Kijewski
Copy link
Collaborator Author

Kijewski commented Jan 27, 2022

Right now the use of serde_json is expected at it's errors are caught in a nice way, but I guess serde_qs is equally as useful. Well, at least I am using serde_qs to generate URLs.

Right now I am formatting the query string outside of render(), and supply it in a field. But I would like it if I could use serde_qs::to_string() directly in the template without resorting to using unwrap().

askama_shared/src/error.rs Outdated Show resolved Hide resolved
@djc djc merged commit 85ad2e6 into rinja-rs:main Jan 28, 2022
@Kijewski Kijewski deleted the pr-errors branch January 28, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants