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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 20, 2024
Merged

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

merged 1 commit into from
Sep 20, 2024

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Sep 5, 2024

Checklist

@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
@@ -130,34 +130,34 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) {
if ($share->getShareType() === IShare::TYPE_USER) {
// 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

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 rakekniven, a team, skjnldsv, yemkareems and provokateurin 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

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

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

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 <[email protected]>

fix: tests

Signed-off-by: Git'Fellow <[email protected]>

fix: Fix tests

Signed-off-by: Git'Fellow <[email protected]>
@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

/backport to stable30

@solracsf solracsf merged commit 2ba3f7e into master Sep 20, 2024
173 of 174 checks passed
@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

@provokateurin provokateurin provokateurin approved these changes

@rakekniven rakekniven rakekniven approved these changes

@susnux susnux susnux approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv was automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems was 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 l10n and translations
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