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

Move hardcoded links from translation to templates #29500

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

Conversation

delvh
Copy link
Member

@delvh delvh commented Feb 29, 2024

Now, we can more easily change links as changing a link no longer invalidates all translations in other languages.

@delvh delvh added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 29, 2024
@delvh delvh self-assigned this Feb 29, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 29, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 29, 2024
@delvh delvh mentioned this pull request Feb 29, 2024
@@ -176,7 +176,7 @@ string.desc = Z - A

[error]
occurred = An error occurred
report_message = If you believe that this is a Gitea bug, please search for issues on <a href="https://github.com/go-gitea/gitea/issues" target="_blank">GitHub</a> or open a new issue if necessary.
report_message = If you believe that this is a Gitea bug, please search for issues on %[1]sGitHub%[2]s or open a new issue if necessary.
Copy link
Member

@lunny lunny Feb 29, 2024

Choose a reason for hiding this comment

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

Why not the whole a link? GitHub has no translation for other languages.

Copy link
Member

Choose a reason for hiding this comment

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

See #29500 (comment) why this is a bad idea.

@silverwind
Copy link
Member

The only issue with whole links is that the translator looses context and just sees %s, but I guess it's a minor one.

@delvh
Copy link
Member Author

delvh commented Feb 29, 2024

Yeah, I've thought about that as well.
My best idea to combat this was not to introduce any spaces there, so that everyone knows wait, why is the text directly surrounded by these parameters?

@silverwind
Copy link
Member

I think the last change with full links should be reverted. A translator wouldn't know what to do with Gitea runs anywhere %s can compile for and not even the translation key gives any hint.

@wolfogre
Copy link
Member

wolfogre commented Mar 1, 2024

One more question, what if we need different links for different languages?

I mean https://docs.gitea.com/installation/install-from-binary for locale_en-US.ini while https://docs.gitea.com/zh-cn/installation/install-from-binary for locale_zh-CN.ini.

I know we never did that, and most docs missed localization. But when we want to use a different link for a specific language, it is easy to apply or revert it: just update the translations on Crowdin instead of submitting a PR to update golang or template codes.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 1, 2024

Sorry but I do not think it could do so directly at the moment.

All existing non-English translations would break, lead to incorrect template sentences.

The reason is the same: these keys/values would never be updated at crowdin side. Unless this could be done first: #27700 (comment) , or use a new key for them.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 1, 2024
@delvh
Copy link
Member Author

delvh commented Mar 1, 2024

I think the last change with full links should be reverted. A translator wouldn't know what to do with Gitea runs anywhere %s can compile for and not even the translation key gives any hint.

@lunny I'll let you discuss this with @silverwind. Tell me once you've reached an agreement

@delvh
Copy link
Member Author

delvh commented Mar 1, 2024

But when we want to use a different link for a specific language, it is easy to apply or revert it: just update the translations on Crowdin instead of submitting a PR to update golang or template codes.

I know we never did that, and most docs missed localization.

Hmm… Do we want to support hypotheticals?
After all, if it was actually done, I'd agree.
However, I'd say we have the opposite case:
No one dares to change translations containing an outdated link as they know it will trigger the process of invalidating all these other translations.
And so far, the link has changed a lot more often than it was translated into another URL.
And as such, I'm more in favor of the proposed approach.

@delvh
Copy link
Member Author

delvh commented Mar 1, 2024

All existing non-English translations would break, lead to incorrect template sentences.

I don't understand, aren't outdated translations automatically invalidated and removed from the active pool of "having a translation"?

#29500 (comment)

I don't see how this blocks this PR?
I didn't change any key itself, only the number of parameters within them.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 1, 2024

All existing non-English translations would break, lead to incorrect template sentences.

I don't understand, aren't outdated translations automatically invalidated and removed from the active pool of "having a translation"?

IIRC no. So that's the real problem. See #27700 (comment), at that time, the "outdated" didn't get "invalidated" or "removed"

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/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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