Skip to content

Add a tool to detect possible unused language keys and remove those which can be confirmed #34737

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 16, 2025

When do some translations, I found some keys have never been used. So that I wrote a tool to help to avoid unused language keys.

  • A new tool to detect unused language keys has been introduced. How to use
make check-locales

the tool will search all .go(except test go files) or .tmpl files to find "$key" to detect whether the language keys are being used. This cannot cover all the situations, i.e. some keys are composited dynamically. So that manually check is necessary. There is a whitelist with glob syntax to manually skip such a situation.

  • The tool also be used in the CI for pull request to check whether unused language keys are missed.
  • This PR also fixes all unused and misused language keys.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 16, 2025
@lunny lunny force-pushed the lunny/remove_unused_translation_key branch from 183f1cd to 760bf0d Compare June 16, 2025 21:48
@lunny lunny marked this pull request as ready for review June 16, 2025 21:48
@lunny lunny added this to the 1.25.0 milestone Jun 16, 2025
@hiifong
Copy link
Member

hiifong commented Jun 18, 2025

I think you can add this command to the Makefile.

@silverwind
Copy link
Member

silverwind commented Jun 18, 2025

Can you move the tool to tools directory? It fits better there.

Also maybe add a convenience make target for it.

@wxiaoguang
Copy link
Contributor

  1. I believe it can be optimized to finish in a few seconds (at most)
  2. It still lacks the ability to check missing keys like Tr("the.key.is.missing.in.locale")
  3. We need to declare these non-static keys at the place where they are used, but not make a tool to "know everything"
  4. We should never introduce any new top-level single-word keys like Tr("error"), it is unsearchable and make it impossible to maintain.

@silverwind
Copy link
Member

We should never introduce any new top-level single-word keys like Tr("error"), it is unsearchable and make it impossible to maintain.

I disagree, sometimes you have simple words to translate, and top-level is ideal for them. Unsearchability is a tooling issue.

@silverwind
Copy link
Member

silverwind commented Jun 18, 2025

I believe it can be optimized to finish in a few seconds (at most)

Yes, I suppose this is because it serially walks the files while such a operation could and should be parallelized (maybe with limited concurrency to stay within OS open file number limits).

@wxiaoguang
Copy link
Contributor

We should never introduce any new top-level single-word keys like Tr("error"), it is unsearchable and make it impossible to maintain.

I disagree, sometimes you have simple words to translate, and top-level is ideal for them. Unsearchability is a tooling issue.

It's impossible to build a correct tool to resolve the "tooling issue". If you think yes, please show a feasible solution.

@wxiaoguang
Copy link
Contributor

I believe it can be optimized to finish in a few seconds (at most)

Yes, I suppose this is because it serially walks the files while such a operation could and should be parallelized (maybe with limited concurrency to stay within OS open file number limits).

I think it's purely a algorithm problem, no parallelization is needed ......

@silverwind
Copy link
Member

I believe it can be optimized to finish in a few seconds (at most)

Yes, I suppose this is because it serially walks the files while such a operation could and should be parallelized (maybe with limited concurrency to stay within OS open file number limits).

I think it's purely a algorithm problem, no parallelization is needed ......

Yeah, I was just refering to filepath.WalkDir which is presumably a serial operation, but likely still fast enough.

@silverwind
Copy link
Member

silverwind commented Jun 18, 2025

We should never introduce any new top-level single-word keys like Tr("error"), it is unsearchable and make it impossible to maintain.

I disagree, sometimes you have simple words to translate, and top-level is ideal for them. Unsearchability is a tooling issue.

It's impossible to build a correct tool to resolve the "tooling issue". If you think yes, please show a feasible solution.

Could introduce a prefix to all translations like tr-error which makes the strings greppable via tr-error\b for example.

@TheFox0x7
Copy link
Contributor

TheFox0x7 commented Jun 18, 2025

I skimmed through this and if I'm understanding correctly it's looking through all files for each key.
Why not inverse this and collect used keys first, then compare them with the list?

something similar in shell (fish but I don't think any specific syntax from it is used). bare in mind it probably is missing keys as it's a quick PoC.

rg "TrN? \"([\w._]*)\"" -o --no-filename --no-line-number -r '$1' >keys #find all Tr(N)s from templates
rg "Tr?N\(\"(.*)\"\)" -o --no-filename --no-line-number -r '$1' >> keys # find all Tr(N)s from go files
sort keys | uniq > keys_in_code # sort for comparison
rg "(.*) = .*" options/locale/locale_en-US.ini -N -r '$1' | sort |uniq > keys_in_locale # strip to keys and sort
comm keys_in_locale keys_in_code -32 # list every key in locale which wasn't in code

Disregard - this does not take locale being in ini format so it's wrong.

@wxiaoguang
Copy link
Contributor

I skimmed through this and if I'm understanding correctly it's looking through all files for each key.
Why not inverse this and collect used keys first, then compare them with the list?

The challenge is like this:

key := "k1"
if ... {
    key = "k2"
}
ctx.Locale.Tr(key)

@TheFox0x7
Copy link
Contributor

Ah. True, that complicates things.

@lunny
Copy link
Member Author

lunny commented Jun 18, 2025

Can you move the tool to tools directory? It fits better there.

Also maybe add a convenience make target for it.

Another locale tool, backport-locale, is located under the build directory. Therefore, I think it makes sense to keep them together. How about moving the entire build directory under tools, as tools/build?

@lunny
Copy link
Member Author

lunny commented Jun 18, 2025

OK. I pushed a commit. Now it will take about 15s so that I also introduced it in CI for pull request.

@lunny
Copy link
Member Author

lunny commented Jun 18, 2025

  1. I believe it can be optimized to finish in a few seconds (at most)

improve, now it's about 15s.

  1. It still lacks the ability to check missing keys like Tr("the.key.is.missing.in.locale")

Yes. This tool just check unused keys but not check missed keys.

  1. We need to declare these non-static keys at the place where they are used, but not make a tool to "know everything"
  2. We should never introduce any new top-level single-word keys like Tr("error"), it is unsearchable and make it impossible to maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code modifies/internal modifies/translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants