Skip to content
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

Refactor i18n, use Locale to provide i18n/translation related functions #18648

Merged
merged 10 commits into from
Feb 8, 2022

Conversation

wxiaoguang
Copy link
Contributor

This PR:

  • remove unnecessary web context data fields, and unify the i18n/translation related functions to Locale
  • in development, show an error if a translation key is missing
  • remove the unnecessary loops for _, lang := range translation.AllLangs() for every request, which improves the performance slightly
  • use ctx.Locale.Language() instead of ctx.Data["Lang"].(string)
  • add more comments about how the Locale/LangType fields are used

@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Feb 7, 2022
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 7, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 7, 2022
wxiaoguang and others added 3 commits February 7, 2022 17:59
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
"AllLangs": translation.AllLangs(),
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
Copy link
Member

Choose a reason for hiding this comment

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

Was this removal intended? There are still other places which set it and head_navbar.tmpl uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The install page do not need it, I have searched the code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

install.tmpl only includes head.tmpl, which says:

		{{if not .PageIsInstall}}
			<div class="ui top secondary stackable main menu following bar light">
				{{template "base/head_navbar" .}}
			</div><!-- end bar -->
		{{end}}

Copy link
Contributor

@singuliere singuliere left a comment

Choose a reason for hiding this comment

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

It would be useful to attach a screenshot of the install page rendered with this commit to prove the removal of the CurrentURL, Language, Lang fields are of no consequence since there is no test to verify that.

@GiteaBot 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 Feb 7, 2022
@wxiaoguang
Copy link
Contributor Author

It would be useful to attach a screenshot of the install page rendered with this commit to prove the removal of the CurrentURL, Language, Lang fields are of no consequence since there is no test to verify that.

I have tested them, including issue activity pages, etc, screenshot shows no difference.

@GiteaBot 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 Feb 7, 2022
@@ -25,17 +25,18 @@ type Locale interface {

// LangType represents a lang type
type LangType struct {
Copy link
Contributor

@zeripath zeripath Feb 7, 2022

Choose a reason for hiding this comment

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

Maybe a crazy idea here but can't we unify this with type locale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's feasible, and we could make a better locale struct.

In fact I had a plan to refactor all {{TimeSince .StartTime $.i18n.Lang}} to {{i18nUtil.TimeSince .StartTime}} to make code more simple and clear.

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I think LangType and locale could be unified.

otherwise LGTM.

@codecov-commenter
Copy link

Codecov Report

Merging #18648 (0ecd380) into main (9712f7d) will increase coverage by 0.36%.
The diff coverage is 55.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18648      +/-   ##
==========================================
+ Coverage   46.31%   46.67%   +0.36%     
==========================================
  Files         846      846              
  Lines      121244   121266      +22     
==========================================
+ Hits        56155    56606     +451     
+ Misses      58271    57790     -481     
- Partials     6818     6870      +52     
Impacted Files Coverage Δ
models/migrate.go 40.00% <ø> (+0.73%) ⬆️
models/unittest/testdb.go 15.87% <0.00%> (-0.26%) ⬇️
modules/context/context.go 64.48% <ø> (-0.39%) ⬇️
routers/install/install.go 0.67% <0.00%> (+0.01%) ⬆️
routers/web/repo/blame.go 0.00% <0.00%> (ø)
routers/web/repo/issue_content_history.go 0.00% <0.00%> (ø)
services/forms/repo_form.go 42.10% <ø> (ø)
models/issue_label.go 65.05% <25.00%> (-0.92%) ⬇️
services/cron/tasks.go 42.30% <40.00%> (-0.12%) ⬇️
modules/translation/translation.go 61.66% <83.33%> (+4.37%) ⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8422b1c...0ecd380. Read the comment docs.

@wxiaoguang wxiaoguang merged commit a60e8be into go-gitea:main Feb 8, 2022
@wxiaoguang wxiaoguang deleted the refactor-i18n branch February 8, 2022 03:02
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 8, 2022
* giteaofficial/main: (28 commits)
  Added auto-save whitespace behavior if it changed manually (go-gitea#15566)
  Support custom ACME provider (go-gitea#18340)
  Refactor i18n, use Locale to provide i18n/translation related functions (go-gitea#18648)
  Only request write when necessary (go-gitea#18657)
  [skip ci] Updated translations via Crowdin
  Add separate SSH_USER config option (go-gitea#17584)
  Be more lenient with label colors (go-gitea#17752)
  remove redundant call to UpdateRepoStats during migration (go-gitea#18591)
  more repo dump/restore tests, including pull requests (go-gitea#18621)
  No longer show the db-downgrade SQL in production (go-gitea#18653)
  Fix the missing i18n key for update checker (go-gitea#18646)
  Update gitea-vet (go-gitea#18640)
  Future proof for 1.18 (go-gitea#18644)
  Add `contrib/upgrade.sh` (go-gitea#18286)
  If rendering has failed due to a net.OpError stop rendering (go-gitea#18642)
  Delete old git.NewCommand() and use it as git.NewCommandContext() (go-gitea#18552)
  Update JS dependencies (go-gitea#18636)
  fix commits_list_small.tmpl (go-gitea#18641)
  Fix `make fmt` and `make fmt-check` (go-gitea#18633)
  Frontport of changelog for v1.16.1 (go-gitea#18615)
  ...
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…ns (go-gitea#18648)

* remove unnecessary web context data fields, and unify the i18n/translation related functions to `Locale`
* in development, show an error if a translation key is missing
* remove the unnecessary loops `for _, lang := range translation.AllLangs()` for every request, which improves the performance slightly
* use `ctx.Locale.Language()` instead of `ctx.Data["Lang"].(string)`
* add more comments about how the Locale/LangType fields are used
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants