[Proposal] Clean up and refactor the template helper functions #23328
Labels
type/proposal
The new feature has not been accepted yet but needs to be discussed first.
type/refactoring
Existing code has been cleaned up. There should be no new functionality.
type/summary
This issue aggregates a bunch of other issues
Comments
wxiaoguang
added
type/feature
yardenshoham
added
type/refactoring
This was referenced Apr 4, 2023
techknowlogick
pushed a commit
that referenced
this issue
Apr 7, 2023
The first step of #23328 This PR cleans: 1. Dead function like `SubStr` 2. Unnecessary function like `UseHTTPS`, it should be provided by `window.origin` 3. Duplicate function like `IsShowFullName`, there was already a `DeafultShowFullName` I have searched these removed functions globally, no use in code.
lunny
pushed a commit
that referenced
this issue
Apr 7, 2023
One of the proposals in #23328 This PR introduces a simple expression calculator (templates/eval/eval.go), it can do basic expression calculations. Many untested template helper functions like `Mul` `Add` can be replaced by this new approach. Then these `Add` / `Mul` / `percentage` / `Subtract` / `DiffStatsWidth` could all use this `Eval`. And it provides enhancements for Golang templates, and improves readability. Some examples: ---- * Before: `{{Add (Mul $glyph.Row 12) 12}}` * After: `{{Eval $glyph.Row "*" 12 "+" 12}}` ---- * Before: `{{if lt (Add $i 1) (len $.Topics)}}` * After: `{{if Eval $i "+" 1 "<" (len $.Topics)}}` ## FAQ ### Why not use an existing expression package? We need a highly customized expression engine: * do the calculation on the fly, without pre-compiling * deal with int/int64/float64 types, to make the result could be used in Golang template. * make the syntax could be used in the Golang template directly * do not introduce too much complex or strange syntax, we just need a simple calculator. * it needs to strictly follow Golang template's behavior, for example, Golang template treats all non-zero values as truth, but many 3rd packages don't do so. ### What's the benefit? * Developers don't need to add more `Add`/`Mul`/`Sub`-like functions, they were getting more and more. Now, only one `Eval` is enough for all cases. * The new code reads better than old `{{Add (Mul $glyph.Row 12) 12}}`, the old one isn't familiar to most procedural programming developers (eg, the Golang expression syntax). * The `Eval` is fully covered by tests, many old `Add`/`Mul`-like functions were never tested. ### The performance? It doesn't use `reflect`, it doesn't need to parse or compile when used in Golang template, the performance is as fast as native Go template. ### Is it too complex? Could it be unstable? The expression calculator program is a common homework for computer science students, and it's widely used as a teaching and practicing purpose for developers. The algorithm is pretty well-known. The behavior can be clearly defined, it is stable.
jolheiser
pushed a commit
that referenced
this issue
Apr 7, 2023
One of the steps in #23328 Before there were 3 different but similar functions: dict/Dict/mergeinto The code was just copied & pasted, no test. This PR defines a new stable `dict` function, it covers all the 3 old functions behaviors, only +160 -171 Future developers do not need to think about or guess the different dict functions, just use one: `dict` Why use `dict` but not `Dict`? Because there are far more `dict` than `Dict` in code already ......
lunny
pushed a commit
that referenced
this issue
Apr 8, 2023
…ror messages (#23982) Follow #23328 Major changes: * Group the function in `templates/help.go` by their purposes. It could make future work easier. * Remove the `Printf` helper function, there is already a builtin `printf`. * Remove `DiffStatsWidth`, replace with `Eval` in template * Rename the `NewTextFuncMap` to `mailSubjectTextFuncMap`, it's for subject text template only, no need to make it support HTML functions. ---- And fine tune template error messages, to make it more friendly to developers and users.   --------- Co-authored-by: silverwind <[email protected]>
silverwind
added a commit
that referenced
this issue
Apr 22, 2023
Follow #23328 The improvements: 1. The `contains` functions are covered by tests 2. The inconsistent behavior of `containGeneric` is replaced by `StringUtils.Contains` and `SliceUtils.Contains` 3. In the future we can move more help functions into XxxUtils to simplify the `helper.go` and reduce unnecessary global functions. FAQ: 1. Why it's called `StringUtils.Contains` but not `strings.Contains` like Golang? Because our `StringUtils` is not Golang's `strings` package. There will be our own string functions. --------- Co-authored-by: silverwind <[email protected]>
|
Almost done! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Feature Description
Gitea is heavily using Go HTML Template, so it's worth to make the template system more maintainable.
There are some strange smells in
template/helper.go:Dict/dict/mergeinto: actually we can have only one unique "dict"(map) maintaining function.Add,Mul,percentage, etc, they could be replaced by a more genericEvallike:{{Eval "(" $a "+" $b ")" "/" 100}}Printfvsprintf: only builtinprintfis enoughPrintf, improve template error messages #23982Context, it's better to introduce a general helper map or struct, then old code{{RenderEmoji $.Context ...}}could be improved to{{.CtxFunc.RenderEmoji ...}}containGenericvscontain: clarify their behaviorTODO:
"SettingUI": &setting.UIto expose all UI settings to templatesps: only do big refactoring after 1.19.0 or even 1.19.1 gets released, to prevent from making backport difficult.
The text was updated successfully, but these errors were encountered: