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

Fail if template references unknown filter #908

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

itowlson
Copy link
Contributor

The template system is intentionally forgiving of parse errors, because a template might contain text that confuses the Liquid parser but which an author wants to be included anyway. However, this can disguise genuine authoring errors. This particular one bit me when accidentally using the redirect template with a previous version of Spin - it appeared to succeed but actually failed on a new filter and dumped the unsubstituted template into my project. With this fix, I need never be confused again.

Longer term, we perhaps need more robust mechanisms for template authors to exclude files from parsing, after which we can banhammer actual parse failures.

Signed-off-by: itowlson ivan.towlson@fermyon.com

@itowlson itowlson force-pushed the templates-fail-better branch 5 times, most recently from a1a6205 to c7305f4 Compare November 28, 2022 23:35
let err_lines = err_str.lines().collect_vec();

// They should use typed errors like we, er, don't
match err_lines.first() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no need to collect the lines into a Vec if you only care about the first one.

Suggested change
match err_lines.first() {
match err_str.lines().next() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh thanks!

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson merged commit b88a63a into fermyon:main Nov 29, 2022
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.

4 participants