Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Improve string template#9

Open
ReverseTM wants to merge 10 commits intotarantool:master from
ReverseTM:strings-template
Open

Improve string template #9
ReverseTM wants to merge 10 commits intotarantool:master from
ReverseTM:strings-template

Conversation

@ReverseTM
Copy link
Collaborator

@ReverseTM ReverseTM commented Jul 24, 2025 •
edited
Loading

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.

Copy link
Collaborator

@hackallcode hackallcode left a 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)
Copy link
Collaborator

@hackallcode hackallcode Jul 25, 2025

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.

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance has improved

Copy link
Collaborator

@hackallcode hackallcode Jul 28, 2025

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

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Collaborator

@hackallcode hackallcode left a 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).
Copy link
Collaborator

@hackallcode hackallcode Jul 28, 2025

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

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@hackallcode hackallcode Jul 28, 2025

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

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 30, 2025

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
Подобна структуре для формата `http`, за исключением того,
что поле `format_template` неизменяемое и всегда равняется значению по умолчанию.

#### Фильтры и функции, используемые в шаблонных строках
Copy link
Collaborator

@hackallcode hackallcode Jul 28, 2025

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.

Copy link
Collaborator Author

@ReverseTM ReverseTM Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved

Copy link
Collaborator

@hackallcode hackallcode Jul 28, 2025

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

Comment on lines 363 to 383
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
}
Copy link
Collaborator

@Hoodie-Huuuuu Hoodie-Huuuuu Aug 4, 2025

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.

Suggested change
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
}

Copy link
Collaborator Author

@ReverseTM ReverseTM Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated function

Comment on lines 708 to 744
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{},
},
}
Copy link
Collaborator

@Hoodie-Huuuuu Hoodie-Huuuuu Aug 4, 2025

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.

Copy link
Collaborator Author

@ReverseTM ReverseTM Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@ReverseTM ReverseTM force-pushed the strings-template branch 2 times, most recently from aa95bd9 to eb0d018 Compare August 8, 2025 08:41
yel0k and others added 10 commits August 16, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hackallcode hackallcode hackallcode requested changes

@Hoodie-Huuuuu Hoodie-Huuuuu Hoodie-Huuuuu left review comments

@choseenonee choseenonee choseenonee left review comments

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /