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

Make bounce total number formatted on bounces per list page #387

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@xh3n1
Copy link
Member

xh3n1 commented Aug 6, 2018

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Aug 6, 2018

@xh3n1 Please check that the changed translation string also works with existing translations of other languages which which presumably still include references to the old print formatting characters.

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Aug 15, 2018

@xh3n1 Please try to replace the old string which has now been replaced (line 77) with the new one so that all existing translations are not outdated. E.g. the natural language text has not changed, only the removal of printf formatting, so a regex / bulk search and replace on the translation files should be possible to modify the strings and avoid triggering creation of a 'new' string. That should be done before this PR is merged in order to avoid an unnecessary automated notification to translators.

@@ -139,7 +139,7 @@ msgstr "Sélectionner une autre liste"
#: public_html/lists/admin/listbounces.php:83
#, php-format
#, php-format, php-format
msgid "%d bounces to list %s"
msgid "bounces to list"
msgstr "%d messages rejetés de la liste %s"

This comment has been minimized.

@xh3n1

xh3n1 Aug 16, 2018

Member

@samtuke there are some translated strings like this one, should I remove %d and %s from those as well?

This comment has been minimized.

@samtuke

samtuke Aug 16, 2018

Contributor

@xh3n1 Yes, but only if you can also adapt the existing translations so they are not all invalidated

echo '<p>'.s('%d bounces to list %s', $total, listName($listid)).'</p>';
echo '<p>'.$totalFormatted.' '.s('bounces to list').' \''.listName($listid).'\'</p>';

This comment has been minimized.

@michield

michield Sep 2, 2018

Member

No, this is not a good idea. Sometimes in other languages, things are not in the same order as they are in english. So, is is better to keep the %d and %s in the translated strings, and allow the translators to determine where the %d has to go in the sentence. This may need more explanation for translators, but the code should allow changing the order. It may well be that in eg Chinese, the structure would be "Listname has XX bounces". Actually, while writing that, we should enforce the order eg %1d and %2s, so that "Listname has XX bounces" would become "%2s has %1d bounces"

This comment has been minimized.

@xh3n1

xh3n1 Sep 3, 2018

Member

@michield The reason why we wanted to make this change is that you can't you use number_format in %d format.

This comment has been minimized.

@michield

michield Sep 3, 2018

Member

Then just use %s for the number_format result. That should work fine.

This comment has been minimized.

@xh3n1

xh3n1 Sep 3, 2018

Member

Wouldn't that still notify the translators that "a new string has been added"?

This comment has been minimized.

@michield

michield Sep 4, 2018

Member

Yes, but that can't be avoided, it would happen either way.

@@ -127,7 +127,7 @@ msgstr ""

#: public_html/lists/admin/listbounces.php:83
#, php-format
msgid "%d bounces to list %s"
msgid "bounces to list"

This comment has been minimized.

@michield

michield Sep 2, 2018

Member

Do not edit these files. They are auto-generated on the translation system, and changing them has no effect.

This comment has been minimized.

@samtuke

samtuke Sep 10, 2018

Contributor

It should be possible to reimport them into pootle, so that simple repetitive changes, like from %d to %s, can be done via regex, without having to trouble the translators (see the 'Update all from VCS' button here: https://translate.phplist.org/en/phplist/).

Without making this change, introducing the new number formatting won't work for all non-english translations, as they will be telling PHP to use the from var type. @xh3n1 Will test this approach to see if it works.

replace %d with %s, apply number format and change translation files
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@xh3n1 xh3n1 force-pushed the bounces-number-format branch from 43612b2 to a1f47c0 Sep 11, 2018

@samtuke samtuke merged commit 42b4ae7 into master Nov 29, 2018

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@samtuke samtuke deleted the bounces-number-format branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment