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

Infrastructure: update en_US plural translations workflow #5956

Merged
merged 1 commit into from
Feb 13, 2022

Conversation

FloXire
Copy link
Contributor

@FloXire FloXire commented Feb 5, 2022

Brief overview of PR changes/additions

Add new workflow to automatically update the file containing american english plural sentences (still requires someone to do the pluralization job).

Motivation for adding to Mudlet

This PR should facilitate the associated step of the release process by gradually doing the job and automating the three first sub steps.

Other info (issues closed, discussion etc)

Related to #5858.

I found out that dealing with all the edge cases to fully automate the process with automatic translations would be too complicated so I chose to create a draft PR so that contributors can add their commit with plural forms to it. Tell me if that's fine. Also tell me if the cron frequency is ok or if I should change it.

Two questions:

  • Should I create an issue referencing the draft PR to tell contributors that there are sentences to pluralize ?
  • Sometimes there is no new sentence to pluralize but there are still changes in the generated translations/translated/mudlet_en_US.ts file (to indicate that the location of the sentence in the source file changed). Should I also create a PR in that case (not a draft one) ?

Release post highlight

@FloXire FloXire requested a review from a team as a code owner February 5, 2022 21:37
@add-deployment-links
Copy link

add-deployment-links bot commented Feb 5, 2022

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@mudlet-machine-account mudlet-machine-account added this to the 4.16.0 milestone Feb 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2022

Messages
✔️

PR type: Infrastructure

Generated by 🚫 dangerJS against 04f3d69

@FloXire FloXire changed the title feat: add update en_US plural translations workflow add: update en_US plural translations workflow Feb 5, 2022
@vadi2 vadi2 changed the title add: update en_US plural translations workflow Infrastructure: update en_US plural translations workflow Feb 5, 2022
@vadi2
Copy link
Member

vadi2 commented Feb 5, 2022

Thanks @FloXire! Will review it tomorrow.

Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it's a nice step towards more automation 👍

create a draft PR so that contributors can add their commit with plural forms to it.

Yep that sounds fine. @SlySven do you want to take up the job of adding plural translations? As far as I know we do not have anybody doing this job at the moment, so we need someone to cover the PRs that will be generated.

Also tell me if the cron frequency is ok or if I should change it.

Yep that is fine. Could you just check or close an existing PR, so we only have 1 open on this subject?

Should I create an issue referencing the draft PR to tell contributors that there are sentences to pluralize ?

Not needed, PR is enough.

Sometimes there is no new sentence to pluralize but there are still changes in the generated translations/translated/mudlet_en_US.ts file (to indicate that the location of the sentence in the source file changed). Should I also create a PR in that case (not a draft one) ?

No, let's only create one if there is actual pluralization work to be done. Yes it means the line numbers will be wrong, but that is a very trivial problem we have been ignoring. See here on how to ignore line # changes only.

@SlySven
Copy link
Member

SlySven commented Feb 7, 2022

Yep that sounds fine. @SlySven do you want to take up the job of adding plural translations? As far as I know we do not have anybody doing this job at the moment, so we need someone to cover the PRs that will be generated.

Okay, I don't expect to be overwhelmed by this as the last time I checked there was only 15 or so.

Though that does remind me that that blind Arabic translator did find one place where we didn't have an %n when we should have done (Discord #translation channel 2022/02/01) - so there may be a forthcoming PR that will be able to test this...! 😀

@FloXire
Copy link
Contributor Author

FloXire commented Feb 7, 2022

Could you just check or close an existing PR, so we only have 1 open on this subject?

I'm not sure I understand this correctly. What you want is that if there's already an existing PR I do not create a new one right ?

@SlySven so should I add you as an assignee whenever such a PR is automatically created ?

@vadi2
Copy link
Member

vadi2 commented Feb 7, 2022

What you want is that if there's already an existing PR I do not create a new one right ?

Ideally update it, because there's new content now, right?

@FloXire
Copy link
Contributor Author

FloXire commented Feb 8, 2022

What you want is that if there's already an existing PR I do not create a new one right ?

Ideally update it, because there's new content now, right?

Right I think I can make it work like that using a fixed name branch. Also currently the PR title to update plural form is set to add: .... Would you prefer Infrastructure: ... as it was the case for this PR ?

@vadi2
Copy link
Member

vadi2 commented Feb 8, 2022

Infrastructure, please.

@SlySven
Copy link
Member

SlySven commented Feb 8, 2022

@SlySven so should I add you as an assignee whenever such a PR is automatically created ?

Yeah...

@FloXire FloXire force-pushed the add-update-en-us-plural-workflow branch from 31ffef3 to 98c5ab0 Compare February 13, 2022 14:31
@FloXire FloXire requested review from a team as code owners February 13, 2022 14:35
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@FloXire FloXire force-pushed the add-update-en-us-plural-workflow branch 2 times, most recently from 01315a6 to daadd21 Compare February 13, 2022 15:13
@FloXire FloXire force-pushed the add-update-en-us-plural-workflow branch from daadd21 to 04f3d69 Compare February 13, 2022 15:23
@FloXire
Copy link
Contributor Author

FloXire commented Feb 13, 2022

Alright everything should be fine now but let me know if there are more changes you'd like to see.

Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's give it a spin :)

@vadi2 vadi2 enabled auto-merge (squash) February 13, 2022 17:09
@vadi2 vadi2 disabled auto-merge February 13, 2022 19:29
@vadi2 vadi2 merged commit 3e1594e into Mudlet:development Feb 13, 2022
@vadi2
Copy link
Member

vadi2 commented Feb 13, 2022

Looks like there's no unfinished ones at the moment? https://github.com/Mudlet/Mudlet/runs/5175712644?check_suite_focus=true

@FloXire
Copy link
Contributor Author

FloXire commented Feb 13, 2022

Looks like there's no unfinished ones at the moment? https://github.com/Mudlet/Mudlet/runs/5175712644?check_suite_focus=true

No indeed. In fact, when I was testing it I had to manually introduce new changes in the code in order to see a created PR with the new sentences.

@vadi2
Copy link
Member

vadi2 commented Feb 13, 2022

Awesome. Thank you for your contribution to Mudlet's release process :)

Hopefully we'll see more!

@SlySven
Copy link
Member

SlySven commented Feb 13, 2022

Like I said in #5956 (comment) there are only a few (13 to 15 IIRC) in our code (there is a pair that someone in Discord said they found that needs implementing where a %2 needs converting to a %n):

 QString IrcMessageFormatter::formatNamesMessage(IrcNamesMessage* message, bool isForLua)
 {
     const QString count = QString::number(message->names().count());
     if (isForLua) {
         // lua actually needs the names for parsing, since getting a names
         // list from the UI userModel alone would be limiting to the IRC commands.
         QString nameList = message->names().join(" ");
-        return QObject::tr("! %1 has %2 users: %3").arg(message->channel(), count, nameList);
+        return QObject::tr("! %1 has %n user(s): %2", "", count).arg(message->channel(), nameList);
     } else {
-        return QObject::tr("! %1 has %2 users").arg(message->channel(), count);
+        return QObject::tr("! %1 has %n user(s)", "", count).arg(message->channel());
     }
 }

@FloXire FloXire deleted the add-update-en-us-plural-workflow branch February 19, 2022 22:28
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.

None yet

4 participants