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

Add translator notes to Gettext strings and update makepot task #618

Merged
merged 18 commits into from
Sep 26, 2023

Conversation

unfulvio-godaddy
Copy link
Member

@unfulvio-godaddy unfulvio-godaddy commented Sep 26, 2023

Adds some translators notes requested on #617 via Slack -- Closes #617

About this item:

There are several multi-lines strings. Please provide string in a single line, linguists can adjust the order. Such as
#: rest-api/Controllers/Settings.php:342
msgid ""
"Optional object that defines how the user will interact with and update the "
"setting."
-> "Optional object that defines how the user will interact with and update the setting."

This seems to be the output from our potfile builder, not the source, which truncates lines at 80 chars. I could not find an option in https://github.com/cedaro/grunt-wp-i18n/blob/develop/docs/makepot.md to change this.

We should probably switch to https://developer.wordpress.org/cli/commands/i18n/make-pot/ (something that is recommended also on the Grunt project above) as this other tool does not seem to exhibit the problem. I made a quick test from CLI and runs fine. We could do so with this PR and later update Sake to do the same in plugins. This would require the user to have installed WP CLI (or later have scripts that install the dependency to correctly handle the command).

But I could probably reach the translations team and tell to ignore the line breaks. Nope: they want it without line breaks: #618 (comment)

I have updated the makepot command to switch to WP CLI. I wanted to run a local instance of WP CLI loaded via Composer as dev dependency. However, it turns out that since we bundle Codeception one such dependency already exists and I think it ends up running an older version that doesn't include the i18n command. So I'm a bit stuck there and opted to use a global WP CLI instance - this means if the person running the script doesn't have WP CLI installed, it will fail.

QA

No code change, no tests or changelog needed

@unfulvio-godaddy
Copy link
Member Author

unfulvio-godaddy commented Sep 26, 2023

edit: it appears that the GoLF team prefers not having multi-line strings to translate - I can imagine this could become troublesome in their system, especially if those lines change - perhaps we should try to implement the new potfile generator in the same context as this PR -- see https://godaddy.slack.com/archives/C07TDQWJ1/p1695707752498609?thread_ts=1695360403.201719&cid=C07TDQWJ1

Copy link
Contributor

@itambek-godaddy itambek-godaddy left a comment

Choose a reason for hiding this comment

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

Looking good, although I would tweak the comments here and there.

@@ -333,18 +338,25 @@ public function process_release_payment( $order ) {
if ( $this->get_gateway()->is_credit_card_gateway() ) {

$message = sprintf(
__( '%s %s Pre-Order Release Payment Approved: %s ending in %s (expires %s)', 'woocommerce-plugin-framework' ),
/* translators: Context: A payment is released for a pre-order. Placeholders: %1$s - Payment gateway name, %2$s - Either 'Authorization' or 'Charge' (untranslated), %3$s - Payment method type (e.g. 'PayPal', 'Credit Card', etc.), %4$s - Last four digits of the card or account, %5$s - Expiration date of the payment method */
Copy link
Contributor

Choose a reason for hiding this comment

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

Either 'Authorization' or 'Charge' (untranslated)

Is there a reason why these are untranslated? Perhaps something that was simply initially missed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure unfortunately, and I don't know if there's any legal requirement to keep them as such

woocommerce/class-sv-wc-plugin.php Outdated Show resolved Hide resolved
woocommerce/class-sv-wc-plugin.php Outdated Show resolved Hide resolved
woocommerce/class-sv-wc-framework-bootstrap.php Outdated Show resolved Hide resolved
@itambek-godaddy
Copy link
Contributor

We should probably switch to https://developer.wordpress.org/cli/commands/i18n/make-pot/ (something that is recommended also on the Grunt project above) as this other tool does not seem to exhibit the problem. I made a quick test from CLI and runs fine. We could do so with this PR and later update Sake to do the same in plugins. This would require the user to have installed WP CLI (or later have scripts that install the dependency to correctly handle the command).

@unfulvio-godaddy with GoLF integration in place, this should already be done for us with GitHub Actions. It looks like the actions to update and send files to translations have not yet been set up in this repo, though. I'm going to try going through the rest of the setup steps today, to hopefully finish the integration.

However, since this is an open-source project, I think your suggestion to switch from grunt-wp-i18n to wp-cli i18n makes sense (also, for sake to use the cli tool).

unfulvio-godaddy and others added 7 commits September 26, 2023 17:17
…ateway-integration-subscriptions.php

Co-authored-by: Illimar Tambek (GoDaddy) <[email protected]>
Co-authored-by: Illimar Tambek (GoDaddy) <[email protected]>
Co-authored-by: Illimar Tambek (GoDaddy) <[email protected]>
…ateway-integration-subscriptions.php

Co-authored-by: Illimar Tambek (GoDaddy) <[email protected]>
…odaddy-wordpress/wc-plugin-framework into 617/add-gettext-context-to-strings
@unfulvio-godaddy unfulvio-godaddy changed the title Add translator notes to Gettext strings Add translator notes to Gettext strings and update makepot task Sep 26, 2023
Copy link
Contributor

@itambek-godaddy itambek-godaddy left a comment

Choose a reason for hiding this comment

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

LGTM

woocommerce/class-sv-wc-plugin.php Outdated Show resolved Hide resolved
@itambek-godaddy itambek-godaddy merged commit 63e33ae into master Sep 26, 2023
@unfulvio-godaddy unfulvio-godaddy deleted the 617/add-gettext-context-to-strings branch October 6, 2023 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gettext context for strings
3 participants