-
Notifications
You must be signed in to change notification settings - Fork 63
templates check command: Allow saving rendered samples #5202
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
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
f633e42
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://04c5fe12.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://rei-templatecheck-todisk.matrix-authentication-service-docs.pages.dev |
crates/templates/src/context.rs
Outdated
| pub locale: Option<String>, | ||
|
|
||
| /// A stable identifier for the session that was used in this sample. | ||
| pub session_index: Option<usize>, |
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.
little bit confused on what a session is yet?
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.
Now we call this "browser-session" as part of the rework below
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub struct SampleIdentifier { |
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.
Hmm, this feels like it could be a Vec<(Key, Value)> instead? (with Key probably being &'static str and Value being a String?)
and then the various stages push in that stack, e.g. at the end you get [("lang", "en"), ("user", "alice"), ("index", "1")], and when you reconstruct the file name you get lang:en-user:alice-index:1?
On why a Vec and not a btreemap: it allows duplicate key, and probably that the various levels of encapsulation will have some semantic meaning
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.
Have changed to this approach. Example filename is pages_upstream_oauth2_suggest_link-index:0-browser-session:0-locale:fr.html. Are we happy with that?
Though I am not sure : is allowed on Windows, maybe it'd be better to find something else.
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.
you might be right, = would also work I suppose
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.
Yeah, let's go with this, just on the off chance it actually bites someone.
crates/cli/src/commands/templates.rs
Outdated
| }; | ||
|
|
||
| let locale_dir = if let Some(locale) = &sample_identifier.locale { | ||
| out_dir.join(locale) |
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.
not convinced that we should do one directory per locale
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 it'd make it easier for a German deployment to only care about the German outputs, for example. Maybe there are better ways of doing that.
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.
feels like the kind of post-processing a consumer could do themselves?
64997ef to
16ec04e
Compare
Gives template samples a stable identifier that we can use to track them across different sample render invocations.
Then allow saving rendered samples to a directory using the
--out-dirflag.Note: rendered pages contain unstable data like subresource integrity and asset hashes.
A future PR will add a feature to strip these out so the renders can be compared across different versions of MAS.
Commit-by-commit review.