-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
All three brackets for variables #14465
Conversation
3d3edc1
to
c20d0a2
Compare
c20d0a2
to
03aab6f
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.
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.
Sorry, this message somehow got lost, readding it:
Nice, thanks for adding additional tests also covering the negative cases now.
I think we may want to avoid duplicating the three tests for the two- and three-bracket cases. This will reduce maintenance effort in the future. I think the following should work (but haven't tested it):
['{{', '{{{'].forEach((openingBrackets, index) => {
const closingBrackets = openingBrackets.replace(/{/g, '}'); // Replace each `{` with `}`
it(`should retrieve raw prompt by id (${index + 2} brackets)`, () => {
const rawPrompt = promptService.getRawPrompt('8');
expect(rawPrompt?.template).to.equal(`Hello, ${openingBrackets}name${closingBrackets}`);
});
});
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 am no sure about this proposal. I agree that the duplication on the tests might require maintenance (if we ever change this). However, I am not a huge fan of making tests with loops instead of clearly writing them out. I think if the test ever break, it will be much easier to understand the test cases if they are simple.
Co-authored-by: Philip Langer <planger@eclipsesource.com>
Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
Should be fixed |
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.
Thanks for the revision, works fine now! 👍
fixed #14464
What it does
Allows to use three brackets for variables in prompts. No mixed formats are allows for consistency
How to test
There are automated test.
To manually test,
Review checklist
Reminder for reviewers