-
Notifications
You must be signed in to change notification settings - Fork 4
Improve string template #9
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
base: master
Are you sure you want to change the base?
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.
Thanks for the patch! Good interface, but there are problems in implementation, let's fix them
} | ||
|
||
for i := range count { | ||
generatedValues := make(map[string]any) |
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 patch reduces performance by up to 45%, maybe this is the reason. I suggest trying slices instead of maps. You can also use the CPU profile flag to detect bottlenecks.
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.
Performance has improved
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.
We still have a 10% performance drop, maybe we should make it so there is no map at all
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.
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.
It's gotten better, but there are still some comments.
doc/ru/usage.md
Outdated
#### Фильтры и функции, используемые в шаблонных строках | ||
|
||
Шаблонные строки реализованы с использованием библиотеки `pongo2`, ознакомиться | ||
со всеми доступными фильтрами и функциями можно в репозитории [pongo2](https://github.com/flosch/pongo2). |
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 couldn't find a list of supported functions in this documentation. Give me a link for that list here
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.
https://github.com/flosch/pongo2/blob/master/docs/filters.md
that's all there is
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.
But it's too hard to find in the repository, let's give a direct link to this list in our documentation
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.
Replaced pongo2 with standard library
doc/ru/usage.md
Outdated
@@ -242,6 +244,18 @@ open_ai: | |||
Подобна структуре для формата `http`, за исключением того, | |||
что поле `format_template` неизменяемое и всегда равняется значению по умолчанию. | |||
|
|||
#### Фильтры и функции, используемые в шаблонных строках |
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 paragraph is too small to be anything more than a list item. Let's move this information into the field description.
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.
Moved
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.
It's moved to the end of the section, but it's not that big and only applies to one list item, so we can move it to the list item description
bd2244f
to
227c6fb
Compare
internal/generator/common/utils.go
Outdated
func ExtractValuesFromTemplate(template string) []string { | ||
re := regexp.MustCompile(`{{.*?\.([^\s|}]+).*?}}`) | ||
matches := re.FindAllStringSubmatch(template, -1) | ||
|
||
values := make([]string, 0, len(matches)) | ||
|
||
for _, match := range matches { | ||
values = append(values, match[1]) | ||
} | ||
|
||
return values | ||
} |
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.
We are currently using golang templates, which prohibit accessing fields that contain characters other than these characters [A-Za-z0-9_].
However, we support the generation of columns with these symbols.
For example, the column name specific-name
is considered valid, but template {{ .specific-name }}
is not.
Golang templates provide the ability to access a field like this
{{ index . "specific-name" }}
I propose to support such an appeal.
func ExtractValuesFromTemplate(template string) []string { | |
re := regexp.MustCompile(`{{.*?\.([^\s|}]+).*?}}`) | |
matches := re.FindAllStringSubmatch(template, -1) | |
values := make([]string, 0, len(matches)) | |
for _, match := range matches { | |
values = append(values, match[1]) | |
} | |
return values | |
} | |
func ExtractValuesFromTemplate(template string) []string { | |
// regexp for templates like {{ .name }} | |
reField := regexp.MustCompile(`{{\s*(?:\w+\s+)?\.([^\s|}]+).*?}}`) | |
matchesField := reField.FindAllStringSubmatch(template, -1) | |
// regexp for templates like {{ index . "name-with-specific-symbols" }} | |
reMapKey := regexp.MustCompile(`{{\s*index\s+\.\s+"([^"]+)".*?}}`) | |
matchesMapKeys := reMapKey.FindAllStringSubmatch(template, -1) | |
values := make([]string, 0, max(len(matchesField), len(matchesMapKeys))) | |
for _, match := range matchesField { | |
values = append(values, match[1]) | |
} | |
for _, match := range matchesMapKeys { | |
values = append(values, match[1]) | |
} | |
return values | |
} |
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.
Updated function
testCases := []testCase{ | ||
{ | ||
name: "Empty template", | ||
template: "", | ||
expected: []string{}, | ||
}, | ||
{ | ||
name: "Valid template", | ||
template: "{{ .foo }}.{{.boo}}", | ||
expected: []string{"foo", "boo"}, | ||
}, | ||
{ | ||
name: "Template with functions", | ||
template: "{{ upper .foo | lower }}@{{ .boo }}", | ||
expected: []string{"foo", "boo"}, | ||
}, | ||
{ | ||
name: "Invalid template", | ||
template: "{_{ foo }}", | ||
expected: []string{}, | ||
}, | ||
} |
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.
Based on my comment, you need to add cases to the function under test, at least test cases with the golang template's index
function.
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.
Added
9a2c178
to
aa95bd9
Compare
…g template, updated usage.md, updated CHANGELOG, and improve and columns are sorted at the point of use.
aa95bd9
to
eb0d018
Compare
String templates have been improved, they now support not only a specific pattern, but also the use of other model fields.
In addition, the function to calculate the number of possible values to generate string templates has been modified.