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

[JENKINS-34522] Expose the noun for a item as a task via AlternativeUiTextProvider #2772

Merged
merged 2 commits into from
Mar 8, 2017

Conversation

stephenc
Copy link
Member

@stephenc stephenc commented Mar 2, 2017

See JENKINS-34522

@reviewbybees

@ghost
Copy link

ghost commented Mar 2, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Schedule_a_build_with_parameters=Schedule a build with parameters for {0}
Task_scheduled={0} scheduled
Schedule_a_task=Schedule a {1} for {0}
Schedule_a_task_with_parameters=Schedule a {1} with parameters for {0}
Copy link
Member

Choose a reason for hiding this comment

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

At least delete the now obsolete entries from the other localizations.

Copy link
Member

Choose a reason for hiding this comment

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

BTW core/move-l10n.groovy is the easiest way when keeping other keys.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Fine subject to existence of downstream PR (presumably in branch-api?) demonstrating consumption of the API.

Schedule_a_build_with_parameters=Schedule a build with parameters for {0}
Task_scheduled={0} scheduled
Schedule_a_task=Schedule a {1} for {0}
Schedule_a_task_with_parameters=Schedule a {1} with parameters for {0}
Copy link
Member

Choose a reason for hiding this comment

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

BTW core/move-l10n.groovy is the easiest way when keeping other keys.

@stephenc
Copy link
Member Author

stephenc commented Mar 3, 2017

@jenkinsci/code-reviewers

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

+1 for using core/move-l10n.groovy or whatever to keep the existing translations in the case when the task is actually a build.

@stephenc
Copy link
Member Author

stephenc commented Mar 5, 2017

@oleg-nenashev how can we keep existing and template them at the same time?

@daniel-beck
Copy link
Member

AFAIU @jglick is just referring to the --delete mode that removes obsolete entries without deleting files. These entries are gone, period. No way we can reasonably update them.

@stephenc
Copy link
Member Author

stephenc commented Mar 6, 2017

@daniel-beck correct and as the file only had three entries and all three are gone, it did not make sense to retain files just to keep copyright headers

@oleg-nenashev
Copy link
Member

@daniel-beck @stephenc Actually you could retain them. By just adding a logic to BuildColumn#taskNoun(), which would check instance of the object and return the legacy translation as a fallback for AbstractProjects, where the original translation is valid.

Otherwise I feel incomfortable that we just screw up translations just for delivering shiny features in particular plugins. And with the current translation policy we will never get the 95% of translations back. I would not say I care much, but the "screw up them and go forward" policy stinks bad in regards of the existing translation contributors.

@oleg-nenashev oleg-nenashev dismissed their stale review March 6, 2017 12:39

Dismessed the review since I do not care about the code itself, mostly about the contributor experience

@daniel-beck
Copy link
Member

daniel-beck commented Mar 7, 2017

@oleg-nenashev It seems unnecessary to make the code more complicated to save a minute of translating strings per language, if that. Out of necessity we already have often very complicated code -- no need to add more of that if we don't need to. You're saying you're thinking of the contributors, but does that include those trying to understand the code? If the only reason is the existing translations, rather than localizability more generally, I don't understand your argument at all.

I did #2777, consisting of about 500 strings, in several hours hours (didn't count, and much time was wasted due to #2779, I don't count that). I expect to spend another 1-2 hours on this. Another one or two strings wouldn't even be noticed.

German went from 1289 strings in 1.580 to 1203 strings in 2.49, and most of those (65 out of 86) were lost between 1.656 and 2.0, indicating it was due to how the "agent" terminology change was implemented (probably should have been done differently in hindsight). Otherwise, we don't lose existing translations anywhere near a relevant pace sufficient to make a big deal out of this.


Build_scheduled=Zadanie zosta\u0142o zaplanowane
Schedule_a_build=Dodaj zadanie {0} do kolejki
Schedule_a_build_with_parameters=Uruchom z parametrami dla {0}
Copy link
Member

Choose a reason for hiding this comment

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

@damianszczepanik There's a discussion currently going on in #2772 about the impact of that change on localizations. I'd like to know, would something like this, i.e. no effort done on retaining existing translations, annoy or upset you? Or would you consider it just part of ongoing development?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for delay, will take a look at this

@stephenc
Copy link
Member Author

stephenc commented Mar 8, 2017

Nobody else has expressed an opinion... merging

@stephenc stephenc removed the needs-more-reviews Complex change, which would benefit from more eyes label Mar 8, 2017
@stephenc stephenc merged commit e0603bb into jenkinsci:master Mar 8, 2017
@stephenc stephenc deleted the jenkins-34522 branch March 8, 2017 15:00
oleg-nenashev added a commit to oleg-nenashev/jenkins.io that referenced this pull request Mar 9, 2017
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.

5 participants