Use a new vars template implementation #19156

lunny · 2022-03-21T02:14:57Z

This PR reimplemented Expand in modules/templates/vars and replace all places.

modules/templates/vars/vars.go

models/migrations/migrations_test.go

wxiaoguang · 2022-03-21T06:40:10Z

PR:

Remove most calls to com, improve vars.Expand lunny/gitea#22

(now it has a separate PR 19284)

Changes:

Add com to deny list
Improve vars.Expand
1. it always returns the rendered string (even if there is an error)
2. the key processing is allocation-free, and bytes are written in string batches, faster.
3. support escaped chars, and now we have the ability to extend the syntax in future by the reserved char #
In some cases, always show rendered text to end users, avoid unnecessary 500 erros
Fix some errors in golangci.yml

Maybe the title of this PR could be changed accordingly: Refactor legacy com(common) package and improve the simple string template Expand

.

lunny · 2022-03-21T12:56:24Z

PR:

Remove most calls to com, improve vars.Expand lunny/gitea#22

Changes:

Add com to deny list
This could be another PR.

Improve vars.Expand

it always returns the rendered string (even if there is an error)
I prefer a strict mode.

the key processing is allocation-free, and bytes are written in string batches, faster.
I have changed my code, now it's also allocation-free for key processing.

support escaped chars, and now we have the ability to extend the syntax in future by the reserved char #
Why we need that? The key should not contain {.

In some cases, always show rendered text to end users, avoid unnecessary 500 erros
Hiding the error means we catch the error more later.

Fix some errors in golangci.yml
This could be another PR.

Maybe the title of this PR could be changed accordingly: Refactor legacy com(common) package and improve the simple string template Expand
I will remove all changes about com to keep this PR clean.

wxiaoguang · 2022-03-21T13:04:46Z

PR:

Remove most calls to com, improve vars.Expand lunny/gitea#22

Changes:

Add com to deny list
This could be another PR.

Why can not the refactoring of the legacy com in one PR?

Improve vars.Expand

it always returns the rendered string (even if there is an error)
I prefer a strict mode.

Strict mode doesn't work here because reason 3

the key processing is allocation-free, and bytes are written in string batches, faster.
I have changed my code, now it's also allocation-free for key processing.

support escaped chars, and now we have the ability to extend the syntax in future by the reserved char #
Why we need that? The key should not contain {.

How about there is some {xxx} or {} in README template during rendering? Can you guarantee that the content of README never contain non-var keys?

And The key should not contain {, but users may want to render a separate { or {}.

In some cases, always show rendered text to end users, avoid unnecessary 500 erros
Hiding the error means we catch the error more later.

I never hide errors, I always output them. But in some cases they should not be shown to users in the web, for example, issue URL rendering or README rendering.

Fix some errors in golangci.yml
This could be another PR.

Hmm .... so many PRs, then no PRs.

Maybe the title of this PR could be changed accordingly: Refactor legacy com(common) package and improve the simple string template Expand
I will remove all changes about com to keep this PR clean.

IIRC other maintainers have talked about we should avoid force-pushing when review starts.

modules/context/repo.go

routers/web/repo/issue.go

routers/web/goget.go

modules/templates/vars/vars.go

delvh · 2022-03-30T14:59:49Z

modules/templates/vars/vars.go

+				}
+			}
+
+			if len(match) == 0 {


Why is that an error case? I strongly disagree that not finding the corresponding variable should return an error.

Even if I count that as an error case, it is an unnecessary one as that will already be tested down below with the v, ok := match[template[keyStartPos+1:i]] implicitly.

Even more so, it is implemented incorrectly as it disregards the positional arguments.

modules/templates/vars/vars.go

delvh · 2022-03-30T15:13:41Z

modules/templates/vars/vars.go

+				return "", err
+			}
+		}
+	}


Currently, there is an edge case missing, as an unclosed parentheses at the end (i.e. test {template end as template) would not throw a syntax error.

delvh · 2022-03-30T15:15:54Z

modules/templates/vars/vars_test.go

+			fail:     true,
+		},
+		{
+			template: "expand }, {b} and {c}",


As mentioned above, the other case is currently missing.

delvh · 2022-03-30T15:32:23Z

modules/templates/vars/vars.go

+}
+
+// Expand replaces all variables like {var} to match
+func Expand(template string, match map[string]string, subs ...string) (string, error) {


You could save all the extra trouble with the positional arguments by using something like

Suggested change

func Expand(template string, match map[string]string, subs ...string) (string, error) {

// All positional arguments will be converted into corresponding map entries: match["x"] = subs[x])

func Expand(template string, match map[string]string, subs ...string) (string, error) {

for index, substitution := range subs {

match[strconv.Itoa(index)] = substitution

}

lunny · 2022-04-02T00:47:41Z

replaced by #19284

lunny added the type/enhancement An improvement of existing functionality label Mar 21, 2022

lunny added this to the 1.17.0 milestone Mar 21, 2022

lunny mentioned this pull request Mar 21, 2022

Allow custom default merge message with .gitea/default_merge_message/<merge_style>_TEMPLATE.md #18177

Merged

This comment was marked as outdated.

Sign in to view

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 21, 2022

techknowlogick approved these changes Mar 21, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 21, 2022

wxiaoguang previously approved these changes Mar 21, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 21, 2022

wxiaoguang reviewed Mar 21, 2022

View reviewed changes

modules/templates/vars/vars.go Show resolved Hide resolved

wxiaoguang reviewed Mar 21, 2022

View reviewed changes

models/migrations/migrations_test.go Outdated Show resolved Hide resolved

lunny force-pushed the lunny/vars_expand branch from 3a3b5ee to fe2f1ed Compare March 21, 2022 12:23

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 21, 2022

wxiaoguang removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 21, 2022

delvh reviewed Mar 30, 2022

View reviewed changes

lunny force-pushed the lunny/vars_expand branch from b6026af to 9305493 Compare April 1, 2022 05:46

lunny added 6 commits April 1, 2022 14:21

Use a new vars template implementation

fa841ce

Improve vars Expand

9ef65b3

Remove unrelated changes

b8370d0

Use fmt.Sprintf but vars.Expand when possible

bbd6767

fix bug

3a4d22a

detail error

b540046

lunny force-pushed the lunny/vars_expand branch from 89e414b to b540046 Compare April 1, 2022 06:21

lunny removed this from the 1.17.0 milestone Apr 2, 2022

lunny closed this Apr 2, 2022

lunny deleted the lunny/vars_expand branch April 2, 2022 00:47

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a new vars template implementation #19156

Use a new vars template implementation #19156

lunny commented Mar 21, 2022 •

edited

Loading

This comment was marked as outdated.

wxiaoguang commented Mar 21, 2022 •

edited

Loading

lunny commented Mar 21, 2022

wxiaoguang commented Mar 21, 2022 •

edited

Loading

delvh Mar 30, 2022

delvh Mar 30, 2022

delvh Mar 30, 2022

delvh Mar 30, 2022

lunny commented Apr 2, 2022

-func Expand(template string, match map[string]string, subs ...string) (string, error) {
+// All positional arguments will be converted into corresponding map entries: match["x"] = subs[x])
+func Expand(template string, match map[string]string, subs ...string) (string, error) {
+  for index, substitution := range subs {
+    match[strconv.Itoa(index)] = substitution
+  }

Use a new vars template implementation #19156

Use a new vars template implementation #19156

Conversation

lunny commented Mar 21, 2022 • edited Loading

This comment was marked as outdated.

wxiaoguang commented Mar 21, 2022 • edited Loading

lunny commented Mar 21, 2022

wxiaoguang commented Mar 21, 2022 • edited Loading

delvh Mar 30, 2022

Choose a reason for hiding this comment

delvh Mar 30, 2022

Choose a reason for hiding this comment

delvh Mar 30, 2022

Choose a reason for hiding this comment

delvh Mar 30, 2022

Choose a reason for hiding this comment

lunny commented Apr 2, 2022

lunny commented Mar 21, 2022 •

edited

Loading

wxiaoguang commented Mar 21, 2022 •

edited

Loading

wxiaoguang commented Mar 21, 2022 •

edited

Loading