Skip to content

fix(l10n): Rephrasing some exceptions to be translated#47782

Merged
solracsf merged 1 commit intomasterfrom
reworkShareExceptions
Sep 20, 2024
Merged

fix(l10n): Rephrasing some exceptions to be translated#47782
solracsf merged 1 commit intomasterfrom
reworkShareExceptions

Conversation

@solracsf
Copy link
Member

@solracsf solracsf commented Sep 5, 2024

Checklist

@solracsf solracsf added 3. to review Waiting for reviews feature: language/translations (l10n/i18n) Localization and translation matters labels Sep 5, 2024
@solracsf solracsf added this to the Nextcloud 31 milestone Sep 5, 2024
susnux
susnux approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

New messages seem to be fine.

Still I think we should not translate those errors, as this are not user facing exception (no HintException). Meaning those are developers only and translating them makes debugging log files much harder.

lib/private/Share20/Manager.php
// We expect a valid user as sharedWith for user shares
if (!$this->userManager->userExists($share->getSharedWith())) {
throw new \InvalidArgumentException($this->l->t('SharedWith is not a valid user'));
throw new \InvalidArgumentException($this->l->t('Share recipient is not a valid user'));
Copy link
Contributor

@susnux susnux Sep 5, 2024

Choose a reason for hiding this comment

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

I still think this should have been never be translated.

@susnux susnux requested review from a team, provokateurin, rakekniven, skjnldsv and yemkareems and removed request for a team September 5, 2024 19:39
@susnux susnux added the bug label Sep 5, 2024
@solracsf
Copy link
Member Author

solracsf commented Sep 5, 2024

In the other side, if the user understands the Exception, he may be able to solve the situation without opening an issue... A coin has 2 sides 🫣

provokateurin
provokateurin approved these changes Sep 5, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I agree with @susnux, all non user facing error messages should not be translated. The fix is good though.

@provokateurin
Copy link
Member

provokateurin commented Sep 5, 2024

In the other side, if the user understands the Exception, he may be able to solve the situation without opening an issue... A coin has 2 sides 🫣

They can use a translator for that. The harder part is if we get a translated error message and have to figure out what the original message was. A translator might not give the exact original version and it is much harder to find the source (unless you got a stacktrace).

@susnux
Copy link
Contributor

susnux commented Sep 5, 2024

In the other side, if the user understands the Exception, he may be able to solve the situation without opening an issue...

But the user will not see this, or if they see it they can not fix it because those errors happen only because of invalid API usage -> broken apps or server -> if the user can fix it they are developers and probably should also understand English in our code base. On the otherhand as @provokateurin mentioned for us this makes reading issues / debugging much harder as we need to guess which error message was meant.

@solracsf solracsf added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 6, 2024
@joshtrichards
Copy link
Member

joshtrichards commented Sep 7, 2024

Motivated by translation requirements or not (and whether for users or developers), I quite like the added clarity of these rephrasings. Good stuff!

@susnux

This comment was marked as resolved.

Sign in to view
@rakekniven
Copy link
Member

rakekniven commented Sep 20, 2024

please merge

@joshtrichards

This comment was marked as resolved.

Sign in to view
@solracsf
fix(l10n): Rephrasing some exceptions to be translated
dcbe8da 
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

fix: tests

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

fix: Fix tests

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf solracsf force-pushed the reworkShareExceptions branch from cb810fe to dcbe8da Compare September 20, 2024 16:35
@solracsf solracsf enabled auto-merge September 20, 2024 16:35
@solracsf
Copy link
Member Author

solracsf commented Sep 20, 2024

/backport to stable30

@solracsf solracsf merged commit 2ba3f7e into master Sep 20, 2024
@solracsf solracsf deleted the reworkShareExceptions branch September 20, 2024 16:53
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux approved these changes

@rakekniven rakekniven rakekniven approved these changes

@provokateurin provokateurin provokateurin approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish backport-request bug feature: language/translations (l10n/i18n) Localization and translation matters

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

chore(l10n): Bad english needs to be improved

5 participants

@solracsf @provokateurin @susnux @joshtrichards @rakekniven