-
Notifications
You must be signed in to change notification settings - Fork 515
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
schnauzer: implement v2 template parser #3284
Conversation
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.
Looks fine to me, and test coverage is good!
sources/api/schnauzer/tests/templates/succeeds/04_comments.template
Outdated
Show resolved
Hide resolved
sources/api/schnauzer/tests/templates/succeeds/04_comments.template
Outdated
Show resolved
Hide resolved
^ Addresses feedback from @bcressey |
^ Addresses feedback from @zmrow |
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.
🍰
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.
Looks good.
@@ -0,0 +1,6 @@ | |||
[required-extensions] | |||
beagle = """ | |||
+++ |
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.
Nice test.
} | ||
|
||
#[test] | ||
fn fails_00_invalid_toml() { |
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 appreciate that you have written these as separate tests instead of cycling through directory templates. Separate tests are much better!
.context(error::GrammarParseSnafu)? | ||
.flatten(); | ||
|
||
// If the template succesfully parses, we know there's always exactly one TOML document and one template body. |
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.
Is this true even if front matter is not present? Is front matter always required in a template, or are there cases where front matter would be optional?
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.
As written, the frontmatter is required, but it can be an empty document. In these cases, the template is like this:
+++
template body
We actually have a template in Bottlerocket right now which is a static file, and looks just like this (I can't recall which offhand).
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 will add a testcase that does this.
} | ||
pub use error::Error; |
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.
nit
} | |
pub use error::Error; | |
} | |
pub use error::Error; |
The v2 template format requires all templates to include a frontmatter section which explicitly specifies each setting and helper that is used to render the template.
Comments from @webern had me realize that there was a mismatch in my own understanding of static-text templates and my implementation. The two changes above fix that, and also address a stylistic nit. For what it's worth, we must always require the |
Issue number:
#3133
Description of changes:
The v2 template format requires all templates to include a frontmatter section which explicitly specifies each setting and helper that is used to render the template.
We use
pest
to separate the frontmatter of the document from its body, then serde to parse the frontmatter document.Testing done:
The included tests pass.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.