-
Notifications
You must be signed in to change notification settings - Fork 667
Add YAML template extension #1613
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
Add YAML template extension #1613
Conversation
|
FYI @mareklibra @jtomasek |
|
/cc @spadgett |
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.
What about throwing and Error? IMO, this conflict should be avoided.
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.
This code gets referenced & evaluated very early during Console startup.
Consequently, throwing an Error here will effectively break the whole application - just a blank page with error stack trace shown in the browser console. This is not the behavior we want.
In good browsers, console.warn and console.error provide you with a link to source code (based on source maps) if you want to investigate the issue.
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.
This should be caught at build time.
@vojtechszocs and I talked about fixing this situation by creating unit tests to avoid duplicates and therefore can avoid this type of runtime validation.
278006c to
e7f9901
Compare
|
Fresh rebase on top of #1586 |
e7f9901 to
4875b25
Compare
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.
Just realized this should be YAMLTemplate to match current naming conventions, will update.
4875b25 to
a9e4cac
Compare
|
PR updated & added |
|
/retest |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, spadgett, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR makes
yamlTemplatesinpublic/models/yaml-templates.tsextensible.Note: due to how multi-line template literals work, leading whitespace within the template string should be removed.
It's also possible to pass a
templateName(falls back todefaultif not specified):Attempts to redefine existing templates (based on tuple key
[modelRef, templateName]) will be logged and the conflicting templates will be discarded.