Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Sep 14, 2025

  • Migrate from the current INI format to JSON for translations. JSON is widely supported, including by platforms such as Crowdin and Weblate.
  • Added a check for the json format and duplicate keys

As a next step, we can consider converting to the i18next JSON format, which allows using placeholders like {name} instead of the current %[1]s syntax.

All original ini files have been converted with go run ./tools/convert_locale_ini_json.go

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Sep 14, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 14, 2025
@GiteaBot

This comment was marked as outdated.

@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Sep 14, 2025
@silverwind
Copy link
Member

Very good choice.

@wxiaoguang
Copy link
Contributor

TBH I won't say it is a good choice. Full screen of \"

image

@wxiaoguang

This comment was marked as outdated.

@wxiaoguang
Copy link
Contributor

By the way, you also need to fix

@silverwind
Copy link
Member

Full screen of "

We should eventually remove all HTML from translations which will solve this. There are always ways to interpolate translation into templates without having HTML in the translation itself.

@wxiaoguang
Copy link
Contributor

Full screen of "

We should eventually remove all HTML from translations which will solve this. There are always ways to interpolate translation into templates without having HTML in the translation itself.

The "eventually has been years, many "eventually" things have been years.

@hiifong
Copy link
Member

hiifong commented Sep 15, 2025

I believe switching to TOML would be preferable, as existing translations can be easily converted to the TOML format, Nor will you see a screen full of \"

@silverwind
Copy link
Member

silverwind commented Sep 15, 2025

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

@wxiaoguang
Copy link
Contributor

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

Yes, any well-defined format is a big step.

Still think it should clean up the legacy HTML quotes before migrating, to make the code base clean and avoid low-level mistakes.

@hiifong
Copy link
Member

hiifong commented Sep 15, 2025

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

One drawback of JSON format is that it does not support adding comments.

@wxiaoguang
Copy link
Contributor

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

One drawback of JSON format is that it does not support adding comments.

Agree, that's also a problem.

@silverwind
Copy link
Member

silverwind commented Sep 15, 2025

YAML has comments. Does crowdin support it? Thought I see no pressing need to comment translations.

@hiifong
Copy link
Member

hiifong commented Sep 15, 2025

YAML has comments. Does crowdin support it? Thought I see no pressing need to comment translations.

Support, and also support TOML format.
ref: https://store.crowdin.com/categories/file-formats

@hiifong
Copy link
Member

hiifong commented Sep 15, 2025

ref: #32937
I believe we should add comments in this situation.

@lunny
Copy link
Member Author

lunny commented Sep 15, 2025

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

One drawback of JSON format is that it does not support adding comments.

Why do we need comments for translation keys? Currently, almost none of the existing translations include comments. Although JSON has it's drawback, but the format is parsed well by all kinds of languages and platforms like crowdin, weblate and others.

I have a plan to replace all %s to {user} in the translation strings, I think it's also a suitable time to replace all the HTML tags at the same PR. Otherwise, we have to replace to %s at this PR and replace it again next time.

@hiifong
Copy link
Member

hiifong commented Sep 15, 2025

Still I think JSON is a big step forward over INI. It has clear and well-known escaping rules, and any mistake will be found by any JSON parser (which should run on the CI, and this is trivial to add).

One drawback of JSON format is that it does not support adding comments.

Why do we need comments for translation keys? Currently, almost none of the existing translations include comments. Although JSON has it's drawback, but the format is parsed well by all kinds of languages and platforms like crowdin, weblate and others.

I have a plan to replace all %s to {user} in the translation strings, I think it's also a suitable time to replace all the HTML tags at the same PR. Otherwise, we have to replace to %s at this PR and replace it again next time.

So how do you plan to replace the HTML tags?

@lunny
Copy link
Member Author

lunny commented Sep 15, 2025

I believe switching to TOML would be preferable, as existing translations can be easily converted to the TOML format, Nor will you see a screen full of \"

TOML is also a possible choice but it seems weblate haven't support it.
Replace the html tags one by one manually. There are many examples in the template files.

A drawback of the YAML format is that incorrect indentation can easily lead to invalid or unintended keys. i.e.
error: a
vs

form:
     error: a

@silverwind
Copy link
Member

I think JSON is a good choice. TOML and YAML are both more much complex specs. I think the downside of having to escape " is acceptable.

@lunny lunny added this to the 1.26.0 milestone Sep 25, 2025
@lunny
Copy link
Member Author

lunny commented Sep 27, 2025

It's ready to review again.

@denyskon
Copy link
Member

denyskon commented Oct 2, 2025

This will definitely be problematic with Crowdin. It doesn't simply support changing the file format, this will be as if we uploaded a completely new set of strings IIRC

@lunny
Copy link
Member Author

lunny commented Oct 2, 2025

This will definitely be problematic with Crowdin. It doesn't simply support changing the file format, this will be as if we uploaded a completely new set of strings IIRC

Yes, that the plan. Once this is merged, we can uploaded all the languages files and remove ini format files.

@denyskon
Copy link
Member

denyskon commented Oct 3, 2025

The we'll loose all translations which are created but not proofread ...

@denyskon
Copy link
Member

denyskon commented Oct 3, 2025

Screenshot_20251003_165330_Firefox.jpg

What do we do with cases like these?

@lunny
Copy link
Member Author

lunny commented Oct 3, 2025

Screenshot_20251003_165330_Firefox.jpg

What do we do with cases like these?

Maybe we can approve all the changes manually before the convention.

@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 Oct 4, 2025
@silverwind
Copy link
Member

==> options/locale/locale_cs-CZ.json
❌ Invalid JSON syntax: options/locale/locale_cs-CZ.json

Empty file is not valid json, you need to write {} or detect empty file.

Comment on lines +924 to +938
.PHONY: translation-check
translation-check:
@echo "Checking JSON files in $(LOCALE_DIR)"
@for f in $(LOCALE_FILES); do \
echo "==> $$f"; \
if ! $(NODE_VARS) pnpm exec jsonlint -q $$f > /dev/null 2>&1; then \
echo "❌ Invalid JSON syntax: $$f"; \
exit 1; \
fi; \
if ! $(NODE_VARS) pnpm exec find-duplicated-property-keys -s $$f > /dev/null 2>&1; then \
echo "❌ Duplicate key found in: $$f"; \
exit 1; \
fi; \
done
@echo "✅ All JSON files passed"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sure that we should not introduce more fragile shell scripts into the Makefile.

So many $$ escaping, so many \\ multiple line string

And, and are written by AI? When we started using such characters in Makefile/shell scripts ....

Copy link
Member

@denyskon denyskon left a comment

Choose a reason for hiding this comment

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

Blocking until we've dealt with the Crowdin question

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/dependencies modifies/go Pull requests that update Go code modifies/internal modifies/templates This PR modifies the template files modifies/translation 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.

6 participants