feat(build): improve lint-locale-usage further #8736
Labels
No labels
arch
riscv64
backport/v1.19
backport/v1.20
backport/v1.21/forgejo
backport/v10.0/forgejo
backport/v11.0/forgejo
backport/v12.0/forgejo
backport/v13.0/forgejo
backport/v14.0/forgejo
backport/v7.0/forgejo
backport/v8.0/forgejo
backport/v9.0/forgejo
breaking
bug
bug
confirmed
bug
duplicate
bug
needs-more-info
bug
new-report
bug
reported-upstream
code/actions
code/api
code/auth
code/auth/faidp
code/auth/farp
code/email
code/federation
code/git
code/migrations
code/packages
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
certmagic
dependency
chart.js
dependency
Chi
dependency
Chroma
dependency
citation.js
dependency
codespell
dependency
css-loader
dependency
devcontainers
dependency
dropzone
dependency
editorconfig-checker
dependency
elasticsearch
dependency
enmime
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Git
dependency
git-backporting
dependency
Gitea
dependency
gitignore
dependency
go-ap
dependency
go-enry
dependency
go-gitlab
dependency
Go-org
dependency
go-rpmutils
dependency
go-sql-driver mysql
dependency
go-swagger
dependency
go-version
dependency
go-webauthn
dependency
gocron
dependency
Golang
dependency
goldmark
dependency
goquery
dependency
Goth
dependency
grpc-go
dependency
happy-dom
dependency
Helm
dependency
image-spec
dependency
jsonschema
dependency
KaTeX
dependency
lint
dependency
MariaDB
dependency
Mermaid
dependency
minio-go
dependency
misspell
dependency
Monaco
dependency
PDFobject
dependency
playwright
dependency
postcss
dependency
postcss-plugins
dependency
pprof
dependency
prometheus client_golang
dependency
protobuf
dependency
relative-time-element
dependency
renovate
dependency
reply
dependency
ssh
dependency
swagger-ui
dependency
tailwind
dependency
temporal-polyfill
dependency
terminal-to-html
dependency
tests-only
dependency
text-expander-element
dependency
urfave
dependency
vfsgen
dependency
vite
dependency
Woodpecker CI
dependency
x tools
dependency
XORM
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/branding
forgejo/ci
forgejo/commit-graph
forgejo/documentation
forgejo/furnace cleanup
forgejo/i18n
forgejo/interop
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
Gain
High
Gain
Nice to have
Gain
Undefined
Gain
Very High
good first issue
i18n/backport-stable
impact
large
impact
medium
impact
small
impact
unknown
Incompatible license
issue
closed
issue
do-not-exist-yet
issue
open
manual test
Manually tested during feature freeze
OS
FreeBSD
OS
Linux
OS
macOS
OS
Windows
problem
QA
regression
release blocker
Release Cycle
Feature Freeze
release-blocker
v7.0
release-blocker
v7.0.1
release-blocker
v7.0.2
release-blocker
v7.0.3
release-blocker
v7.0.4
release-blocker
v8.0.0
release-blocker/v9.0.0
run-all-playwright-tests
run-end-to-end-tests
test
manual
test
needed
test
needs-help
test
not-needed
test
present
untested
User research - time-tracker
valuable code
worth a release-note
User research - Accessibility
User research - Blocked
User research - Community
User research - Config (instance)
User research - Errors
User research - Filters
User research - Future backlog
User research - Git workflow
User research - Labels
User research - Moderation
User research - Needs input
User research - Notifications/Dashboard
User research - Rendering
User research - Repo creation
User research - Repo units
User research - Security
User research - Settings (in-app)
No milestone
No project
No assignees
6 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/forgejo!8736
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fogti/llu-unused"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
99f49f7836ec67cce73cec67cce73cf95da43c9bokay, this still needs a test label. it's overall done tho.
List of changes:
models/unit/unit.go, which stores msgids in$Unit.NameKey.locale.Trin templatesTr("msgid-prefix." + SomeFunctionCall())things(the first was actually necessary in order to find more structures which I overlooked)
What's the purpose of
$Unit.DescKey? Like, it appears to be a msgid, but I can't find any usage sites.In
8760af752a, there existed a usage site, but it vanished sometime...feat(build): lint-locale-usage should handle .locale.Tr and Unit{...}to WIP: feat(build): improve lint-locale-usage furtherThis is now basically done, but it needs to be very thoroughly checked, in particular, all changes in
/options(locale files) need to be thoroughly audited to ensure that we don't throw away any necessary translation strings.@ -0,0 +51,4 @@repo.issues.ref_pull_fromrepo.issues.ref_reopening_from# templates/repo/issue/view_content/comments.tmpl: ctx.Locale.Tr (printf "projects.type-%d.display_name" .OldProject.Type)tbh, such dynamic msgid constructions are a pretty bad idea (both because they're basically impossible to lint, and hard to understand what exactly they mean)
This is intertwined enough that I think it makes sense to squash this...
0844216eb20cfc8a77c1WIP: feat(build): improve lint-locale-usage furtherto feat(build): improve lint-locale-usage further0ce5c0e8f2cbc2ac36ea@ -93,7 +93,6 @@"moderation.reporting_failed": "Unable to submit the new abuse report: %v","moderation.reported_thank_you": "Thank you for your report. The administration has been made aware of it.","mail.actions.successful_run_after_failure_subject": "Workflow %[1]s recovered in repository %[2]s","mail.actions.not_successful_run_subject": "Workflow %[1]s failed in repository %[2]s",I think this should have been used in the following code
subject = locale.TrString("mail.actions.not_successful_run", run.Title, run.Repo.FullName())CC @christopher-besch
WIP https://pad.gusted.xyz/s/YbAZQyU7W
@Gusted Thank you!
cbc2ac36ea2d01911207@ -54,3 +54,3 @@subject = locale.TrString("mail.actions.successful_run_after_failure_subject", run.Title, run.Repo.FullName())} else {subject = locale.TrString("mail.actions.not_successful_run", run.Title, run.Repo.FullName())subject = locale.TrString("mail.actions.not_successful_run_subject", run.Title, run.Repo.FullName())Mistake introduced in
386e7f8208cc @christopher-besch
2d0191120768159a247cI think I should put my own INI-parser into this in order to get location information for lines, to then pass those lines to
git blame -Lto find out where it came from, semi-automatically...I think the best way to do that would be to fork https://pkg.go.dev/gopkg.in/ini.v1 to add location tracking.
@ -0,0 +58,4 @@# modules/migration/messenger.go: invocations of Messenger# services/migrations/migrate.go: messenger(...)# *: repo.migrate.*.description (unknown where they come from)repo.migrate.I couldn't really determine where they come from, the migration code is pretty convoluted in regards to msgid construction.
@ -0,0 +19,4 @@arg, err := strconv.Unquote(argLit.Value)if err == nil {// found interesting stringsif strings.HasSuffix(arg, ".") || strings.HasSuffix(arg, "_") {TODO: if ending in a
_, remove the last component.fogti referenced this pull request2025-08-04 00:16:55 +02:00
7cac681aa2875c343f5eIn the last commit, I renamed
repo.tree_path_not_found_torepo.tree_path_not_found..I think the same should be done for
repo.settings.web_hook_name_. (I haven't done that yet because it would be way more disruptive due to affecting more translation strings)@ -0,0 +96,4 @@# modules/migration/messenger.go: invocations of Messenger# services/migrations/migrate.go: messenger(...)# *: repo.migrate.*.description (unknown where they come from)Could you elaborate on your comment here?
there are many entries
repo.migrate.$VENDOR.description, and I have no exact idea where those message ids get used in the source code.Removing that line yields:
{{ctx.Locale.Tr (printf "repo.migrate.%s.description" .Name)}}@ -125,3 +72,1 @@if gotUnexpectedInvoke != nil {handler.OnUnexpectedInvoke(fset, funSel.Sel.NamePos, funSel.Sel.Name, *gotUnexpectedInvoke)}type StringTrieMap map[string]StringTrieIt's up to you if you want to incorporate this hack, but technically the standard library offers a trie and we already use this in
type rememberSecondWriteWriter struct {pos intidx intend intwritecount int}func (n *rememberSecondWriteWriter) Write(p []byte) (int, error) {n.writecount++if n.writecount == 2 {n.idx = n.posn.end = n.pos + len(p)n.pos += len(p)return len(p), io.EOF}n.pos += len(p)return len(p), nil}func (n *rememberSecondWriteWriter) WriteString(s string) (int, error) {n.writecount++if n.writecount == 2 {n.idx = n.posn.end = n.pos + len(s)n.pos += len(s)return len(s), io.EOF}n.pos += len(s)return len(s), nil}// FindEmojiSubmatchIndex returns index pair of longest emoji in a stringfunc FindEmojiSubmatchIndex(s string) []int {loadMap()secondWriteWriter := rememberSecondWriteWriter{}// A faster and clean implementation would copy the trie tree formation in strings.NewReplacer but// we can be lazy here.//// The implementation of strings.Replacer.WriteString is such that the first index of the emoji// submatch is simply the second thing that is written to WriteString in the writer.//// Therefore we can simply take the index of the second write as our first emoji//// FIXME: just copy the trie implementation from strings.NewReplacer_, _ = emptyReplacer.WriteString(&secondWriteWriter, s)// if we wrote less than twice then we never "replaced"if secondWriteWriter.writecount < 2 {return nilAlbeit there is an implementation of a trie in the standard library (
strings). But as already noted in the source code snippet given above, it doesn't appear to be independently usable (especially because we strictly need to match the start of the string).@ -334,17 +231,59 @@ func main() {gotAnyMsgidError := falsefor _, arg := range os.Args[1:] {What do you think about using
flagfor this?Probably a good idea, I'll try.
425a6e496d0c18f6a8edNo clue why the frontend-check fails when trying to sleep enough.
Seen that CI failure a few times before.. you can ignore it.
Hiya, it took a bit of time but all removed translations are now checked (and recorded in the shared pad) and there's no false positives. I will try to review the Go code later this week (feel free to ping me next week if I forget).
@Gusted ping
4c9142eea5bed330fcb2I am not able to spot anything incorrect with the correctness of the code, so I'm mainly focusing on the readability of the code. As this is a linter, that deserves some extra attention.
@ -0,0 +61,4 @@matches := falsematchInsPrefix := ""var multimatches *token.Posif cg == nil {This can be done as first check.
@ -0,0 +72,4 @@multimatches = &comment.Slash}matches = true} else if after, found := strings.CutPrefix(ctxt, commentPrefix+"Suffix "); found {Good use on
strings.CutPrefix:)@ -0,0 +73,4 @@}matches = true} else if after, found := strings.CutPrefix(ctxt, commentPrefix+"Suffix "); found {if matches {It's good, but feels like it can be simplified:
@ -0,0 +112,4 @@ast.Inspect(node, func(n ast.Node) bool {// search for function calls of the form `anything.Tr(any-string-lit, ...)`if call, ok := n.(*ast.CallExpr); ok && len(call.Args) >= 1 {The if else calls don't make it really easy to read what's going on, I think it would make sense to do switch on type.
Inferring types in Go never makes for good looking code.
@ -0,0 +123,4 @@return true}var gotUnexpectedInvoke *intDo you think the
optionalmodule could be used instead of a pointer?it could be, but I didn't want to pull in another module for something so trivial (and pretty common in Go code as well)
actually, I'm surprised that
optionalimplements that feature via a slice instead of just a pointer (one needsnilchecks anyways, what's the point?).Likely because of some early generics limitations.
@ -0,0 +126,4 @@var gotUnexpectedInvoke *intfor _, argNum := range ltf {if len(call.Args) < int(argNum+1) {len(call.Args) <= int(argNum)? Not sure if this should be modified as I expect such condition to not include the equal case and rather belen(call.Args) < int(argNum).note that
call.Args[int(argNum)]is used below, andargNum == 0holds e.g. for.Tr@ -0,0 +144,4 @@}// special case: models/unit/unit.goif strings.HasSuffix(fname, "unit.go") && ident.Name == "Unit" && len(composite.Elts) == 6 {len(composite.Elts) == 6could change in the future and this check wouldn't throw a warning about it, I think it should give a warning or otherwise error.@ -0,0 +173,4 @@ast.Inspect(function.Body, func(n ast.Node) bool {// search for return stmts// TODO: what about nested functions?This specifically targets functions with
llu:returnsTrKeyright? I think you can say that this is disallowed and if there in the future a need arise it can be reevaluated.I did add a note about this to the usage/help text.
@ -127,2 +73,3 @@}type StringTrieMap map[string]StringTriefunc (m *StringTrieMap) Matches(key []string) bool {Similar to the other comment, I think you can drop the pointer as maps are already reference types in Go.
@ -257,3 +114,1 @@tmpl := template.New(fname)tmpl.Funcs(fjTemplates.NewFuncMap())tmplParsed, err := tmpl.Parse(tmplContent2)func ParseAllowedMaskedUsages(fname string, usedMsgids *container.Set[string], allowedMaskedPrefixes *StringTrieMap, chkMsgid func(msgid string) bool) error {Maps are pass-by-reference, there's no need to pass pointers around to maps to my understanding. I think this also could slightly simplify the code as you don't have to dereference them.
@ -264,0 +140,4 @@Err: errors.New(line),}}(*usedMsgids)[line] = struct{}{}usedMsgids.Add(line)(once the map is no longer a pointer).@ -340,2 +304,4 @@gotAnyMsgidError = truefmt.Printf("%s:\tmissing msgid: %s\n", fset.Position(pos).String(), msgid)} else {usedMsgids[msgid] = struct{}{}I see this is already done further up in this function, but it's nicer to do it via
usedMsgids.Add(msgid)to avoid thestruct{}{}hack.bed330fcb29449f9f84cI think the current state is good enough.
Thank you!
great work, @fogti! 🎊
When running
make test-sqlite, I notice quite some error messages which seem to be linked to the translation keys removed by this PR:I imagine those are due to the leftover translation keys in languages other than English? It doesn't look like Weblate is taking care of removing those automatically, is it? #9013
Those errors do not seem to make the overall test process fail, but I would still find it more convenient if they could go away, as they make it harder to look for actual errors.
They are displayed for analysis because logs are not supposed to display errors during test runs but they do no fail tests. They can either be fixed, silenced or ignored. I'm not sure which way is best for these ones but @0ko will know.
@wetneb you found a bug introduced in this PR! These strings weren't supposed to be deleted and now show up on main repo screen. We need to get them back..
No, it is because a template requested these keys and got nothing.
Specifically in INI, it doesn't, but cleanups happen: #9011.
This bug would have been caught by testlogger but it is currently disabled (#2942).
Looking at action logs, there are no other strings that weren't supposed to be removed, at least among the ones seen on pages visited by integration tests.
oh, sorry, should've double-checked that
cddf608cb9didn't remove (all) references to thesearch.*strings.@ -1192,1 +1159,3 @@tree_path_not_found_tag = Path %[1]s doesn't exist in tag %[2]stree_path_not_found.commit = Path %[1]s doesn't exist in commit %[2]stree_path_not_found.branch = Path %[1]s doesn't exist in branch %[2]stree_path_not_found.tag = Path %[1]s doesn't exist in tag %[2]sI should have paid better attention, but this had deleted existing translations. Some are already re-made, but for unmaintained locales I'll try to recover them with the existing tooling after #9041 is unblocked.
Done in #9041.