From 17fc178cb5a68be787cb3fa243aed4cf1abb24a3 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 14 May 2018 15:28:39 +0200 Subject: [PATCH] Correctly translate all forms in tests --- lib/gitlab/i18n/metadata_entry.rb | 11 ++- lib/gitlab/i18n/po_linter.rb | 61 +++++++++++----- lib/gitlab/i18n/translation_entry.rb | 6 +- lib/tasks/gettext.rake | 8 +- spec/lib/gitlab/i18n/metadata_entry_spec.rb | 6 +- spec/lib/gitlab/i18n/po_linter_spec.rb | 73 +++++++++++++++---- .../lib/gitlab/i18n/translation_entry_spec.rb | 6 +- 7 files changed, 126 insertions(+), 45 deletions(-) diff --git a/lib/gitlab/i18n/metadata_entry.rb b/lib/gitlab/i18n/metadata_entry.rb index 35d57459a3d..36fc1bcdcb7 100644 --- a/lib/gitlab/i18n/metadata_entry.rb +++ b/lib/gitlab/i18n/metadata_entry.rb @@ -3,16 +3,25 @@ module Gitlab class MetadataEntry attr_reader :entry_data + # Avoid testing too many plurals if `nplurals` was incorrectly set. + # Based on info on https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html + # which mentions special cases for numbers ending in 2 digits + MAX_FORMS_TO_TEST = 101 + def initialize(entry_data) @entry_data = entry_data end - def expected_plurals + def expected_forms return nil unless plural_information plural_information['nplurals'].to_i end + def forms_to_test + @forms_to_test ||= [expected_forms, MAX_FORMS_TO_TEST].compact.min + end + private def plural_information diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb index ba08ef59776..d8e7269a2c2 100644 --- a/lib/gitlab/i18n/po_linter.rb +++ b/lib/gitlab/i18n/po_linter.rb @@ -36,7 +36,7 @@ module Gitlab end @translation_entries = entries.map do |entry_data| - Gitlab::I18n::TranslationEntry.new(entry_data, metadata_entry.expected_plurals) + Gitlab::I18n::TranslationEntry.new(entry_data, metadata_entry.expected_forms) end nil @@ -84,25 +84,25 @@ module Gitlab end def validate_number_of_plurals(errors, entry) - return unless metadata_entry&.expected_plurals + return unless metadata_entry&.expected_forms return unless entry.translated? - if entry.has_plural? && entry.all_translations.size != metadata_entry.expected_plurals - errors << "should have #{metadata_entry.expected_plurals} "\ - "#{'translations'.pluralize(metadata_entry.expected_plurals)}" + if entry.has_plural? && entry.all_translations.size != metadata_entry.expected_forms + errors << "should have #{metadata_entry.expected_forms} "\ + "#{'translations'.pluralize(metadata_entry.expected_forms)}" end end def validate_newlines(errors, entry) - if entry.msgid_contains_newlines? + if entry.msgid_has_multiple_lines? errors << 'is defined over multiple lines, this breaks some tooling.' end - if entry.plural_id_contains_newlines? + if entry.plural_id_has_multiple_lines? errors << 'plural is defined over multiple lines, this breaks some tooling.' end - if entry.translations_contain_newlines? + if entry.translations_have_multiple_lines? errors << 'has translations defined over multiple lines, this breaks some tooling.' end end @@ -179,16 +179,41 @@ module Gitlab end def numbers_covering_all_plurals - @numbers_covering_all_plurals ||= Array.new(metadata_entry.expected_plurals) do |index| - number_for_pluralization(index) - end + @numbers_covering_all_plurals ||= calculate_numbers_covering_all_plurals end - def number_for_pluralization(counter) - pluralization_result = FastGettext.pluralisation_rule.call(counter) + def calculate_numbers_covering_all_plurals + required_numbers = [] + discovered_indexes = [] + counter = 0 - if pluralization_result.is_a?(TrueClass) || pluralization_result.is_a?(FalseClass) - counter + while discovered_indexes.size < metadata_entry.forms_to_test && counter < Gitlab::I18n::MetadataEntry::MAX_FORMS_TO_TEST + index_for_count = index_for_pluralization(counter) + + unless discovered_indexes.include?(index_for_count) + discovered_indexes << index_for_count + required_numbers << counter + end + + counter += 1 + end + + required_numbers + end + + def index_for_pluralization(counter) + # This calls the C function that defines the pluralization rule, it can + # return a boolean (`false` represents 0, `true` represents 1) or an integer + # that specifies the plural form to be used for the given number + pluralization_result = Gitlab::I18n.with_locale(locale) do + FastGettext.pluralisation_rule.call(counter) + end + + case pluralization_result + when false + 0 + when true + 1 else pluralization_result end @@ -211,11 +236,13 @@ module Gitlab end def validate_unnamed_variables(errors, variables) - if variables.any? { |name| unnamed_variable?(name) } && variables.any? { |name| !unnamed_variable?(name) } + unnamed_variables, named_variables = variables.partition { |name| unnamed_variable?(name) } + + if unnamed_variables.any? && named_variables.any? errors << 'is combining named variables with unnamed variables' end - if variables.select { |variable_name| unnamed_variable?(variable_name) }.size > 1 + if unnamed_variables.size > 1 errors << 'is combining multiple unnamed variables' end end diff --git a/lib/gitlab/i18n/translation_entry.rb b/lib/gitlab/i18n/translation_entry.rb index 13c544aed27..54adb98f42d 100644 --- a/lib/gitlab/i18n/translation_entry.rb +++ b/lib/gitlab/i18n/translation_entry.rb @@ -53,15 +53,15 @@ module Gitlab nplurals > 1 || !has_plural? end - def msgid_contains_newlines? + def msgid_has_multiple_lines? entry_data[:msgid].is_a?(Array) end - def plural_id_contains_newlines? + def plural_id_has_multiple_lines? entry_data[:msgid_plural].is_a?(Array) end - def translations_contain_newlines? + def translations_have_multiple_lines? translation_entries.any? { |translation| translation.is_a?(Array) } end diff --git a/lib/tasks/gettext.rake b/lib/tasks/gettext.rake index efe2997d78e..d52c419d66b 100644 --- a/lib/tasks/gettext.rake +++ b/lib/tasks/gettext.rake @@ -51,7 +51,7 @@ namespace :gettext do end task :updated_check do - # Removeing all pre-translated files speeds up `gettext:find` as the + # Removing all pre-translated files speeds up `gettext:find` as the # files don't need to be merged. `rm locale/*/gitlab.po` @@ -62,15 +62,15 @@ namespace :gettext do changed_files = `git diff --name-only`.lines.map(&:strip) # reset the locale folder for potential next tasks - `git checkout locale` + `git checkout -- locale` if changed_files.include?('locale/gitlab.pot') raise <<~MSG Newly translated strings found, please add them to `gitlab.pot` by running: - bundle exec rake gettext:find; git checkout locale/*/gitlab.po; + bundle exec rake gettext:find; git checkout -- locale/*/gitlab.po; - Then check in the resulting `locale/gitlab.pot` + Then commit and push the resulting changes to `locale/gitlab.pot`. MSG end diff --git a/spec/lib/gitlab/i18n/metadata_entry_spec.rb b/spec/lib/gitlab/i18n/metadata_entry_spec.rb index ab71d6454a9..a399517cc04 100644 --- a/spec/lib/gitlab/i18n/metadata_entry_spec.rb +++ b/spec/lib/gitlab/i18n/metadata_entry_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::I18n::MetadataEntry do - describe '#expected_plurals' do + describe '#expected_forms' do it 'returns the number of plurals' do data = { msgid: "", @@ -22,7 +22,7 @@ describe Gitlab::I18n::MetadataEntry do } entry = described_class.new(data) - expect(entry.expected_plurals).to eq(2) + expect(entry.expected_forms).to eq(2) end it 'returns 0 for the POT-metadata' do @@ -45,7 +45,7 @@ describe Gitlab::I18n::MetadataEntry do } entry = described_class.new(data) - expect(entry.expected_plurals).to eq(0) + expect(entry.expected_forms).to eq(0) end end end diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb index 045916e6dc2..3dbc23d2aaf 100644 --- a/spec/lib/gitlab/i18n/po_linter_spec.rb +++ b/spec/lib/gitlab/i18n/po_linter_spec.rb @@ -1,20 +1,22 @@ require 'spec_helper' require 'simple_po_parser' +# Disabling this cop to allow for multi-language examples in comments +# rubocop:disable Style/AsciiComments describe Gitlab::I18n::PoLinter do let(:linter) { described_class.new(po_path) } let(:po_path) { 'spec/fixtures/valid.po' } - def fake_translation(id:, translation:, plural_id: nil, plurals: []) - data = { msgid: id, msgid_plural: plural_id } + def fake_translation(msgid:, translation:, plural_id: nil, plurals: []) + data = { msgid: msgid, msgid_plural: plural_id } if plural_id [translation, *plurals].each_with_index do |plural, index| - allow(FastGettext::Translation).to receive(:n_).with(plural_id, index).and_return(plural) + allow(FastGettext::Translation).to receive(:n_).with(msgid, plural_id, index).and_return(plural) data.merge!("msgstr[#{index}]" => plural) end else - allow(FastGettext::Translation).to receive(:_).with(id).and_return(translation) + allow(FastGettext::Translation).to receive(:_).with(msgid).and_return(translation) data[:msgstr] = translation end @@ -174,7 +176,7 @@ describe Gitlab::I18n::PoLinter do describe '#validate_entries' do it 'keeps track of errors for entries' do - fake_invalid_entry = fake_translation(id: "Hello %{world}", + fake_invalid_entry = fake_translation(msgid: "Hello %{world}", translation: "Bonjour %{monde}") allow(linter).to receive(:translation_entries) { [fake_invalid_entry] } @@ -204,7 +206,7 @@ describe Gitlab::I18n::PoLinter do describe '#validate_number_of_plurals' do it 'validates when there are an incorrect number of translations' do fake_metadata = double - allow(fake_metadata).to receive(:expected_plurals).and_return(2) + allow(fake_metadata).to receive(:expected_forms).and_return(2) allow(linter).to receive(:metadata_entry).and_return(fake_metadata) fake_entry = Gitlab::I18n::TranslationEntry.new( @@ -226,7 +228,7 @@ describe Gitlab::I18n::PoLinter do it 'validates both singular and plural in a pluralized string when the entry has a singular' do pluralized_entry = fake_translation( - id: 'Hello %{world}', + msgid: 'Hello %{world}', translation: 'Bonjour %{world}', plural_id: 'Hello all %{world}', plurals: ['Bonjour tous %{world}'] @@ -244,7 +246,7 @@ describe Gitlab::I18n::PoLinter do it 'only validates plural when there is no separate singular' do pluralized_entry = fake_translation( - id: 'Hello %{world}', + msgid: 'Hello %{world}', translation: 'Bonjour %{world}', plural_id: 'Hello all %{world}' ) @@ -256,7 +258,7 @@ describe Gitlab::I18n::PoLinter do end it 'validates the message variables' do - entry = fake_translation(id: 'Hello', translation: 'Bonjour') + entry = fake_translation(msgid: 'Hello', translation: 'Bonjour') expect(linter).to receive(:validate_variables_in_message) .with([], 'Hello', 'Bonjour') @@ -266,7 +268,7 @@ describe Gitlab::I18n::PoLinter do it 'validates variable usage in message ids' do entry = fake_translation( - id: 'Hello %{world}', + msgid: 'Hello %{world}', translation: 'Bonjour %{world}', plural_id: 'Hello all %{world}', plurals: ['Bonjour tous %{world}'] @@ -309,7 +311,7 @@ describe Gitlab::I18n::PoLinter do end describe '#validate_translation' do - let(:entry) { fake_translation(id: 'Hello %{world}', translation: 'Bonjour %{world}') } + let(:entry) { fake_translation(msgid: 'Hello %{world}', translation: 'Bonjour %{world}') } it 'succeeds with valid variables' do errors = [] @@ -330,7 +332,7 @@ describe Gitlab::I18n::PoLinter do end it 'adds an error message when translating fails when translating with context' do - entry = fake_translation(id: 'Tests|Hello', translation: 'broken') + entry = fake_translation(msgid: 'Tests|Hello', translation: 'broken') errors = [] expect(FastGettext::Translation).to receive(:s_) { raise 'broken' } @@ -341,7 +343,7 @@ describe Gitlab::I18n::PoLinter do end it "adds an error when trying to translate with incorrect variables when using unnamed variables" do - entry = fake_translation(id: 'Hello %s', translation: 'Hello %d') + entry = fake_translation(msgid: 'Hello %s', translation: 'Hello %d') errors = [] linter.validate_translation(errors, entry) @@ -350,13 +352,55 @@ describe Gitlab::I18n::PoLinter do end it "adds an error when trying to translate with named variables when unnamed variables are expected" do - entry = fake_translation(id: 'Hello %s', translation: 'Hello %{thing}') + entry = fake_translation(msgid: 'Hello %s', translation: 'Hello %{thing}') errors = [] linter.validate_translation(errors, entry) expect(errors.first).to start_with("Failure translating to en") end + + it 'tests translation for all given forms' do + # Fake a language that has 3 forms to translate + fake_metadata = double + allow(fake_metadata).to receive(:forms_to_test).and_return(3) + allow(linter).to receive(:metadata_entry).and_return(fake_metadata) + entry = fake_translation( + msgid: '%d exception', + translation: '%d uitzondering', + plural_id: '%d exceptions', + plurals: ['%d uitzonderingen', '%d uitzonderingetjes'] + ) + + # Make each count use a different index + allow(linter).to receive(:index_for_pluralization).with(0).and_return(0) + allow(linter).to receive(:index_for_pluralization).with(1).and_return(1) + allow(linter).to receive(:index_for_pluralization).with(2).and_return(2) + + expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 0).and_call_original + expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 1).and_call_original + expect(FastGettext::Translation).to receive(:n_).with('%d exception', '%d exceptions', 2).and_call_original + + linter.validate_translation([], entry) + end + end + + describe '#numbers_covering_all_plurals' do + it 'can correctly find all required numbers to translate to Polish' do + # Polish used as an example with 3 different forms: + # 0, all plurals except the ones ending in 2,3,4: Kotów + # 1: Kot + # 2-3-4: Koty + # So translating with [0, 1, 2] will give us all different posibilities + fake_metadata = double + allow(fake_metadata).to receive(:forms_to_test).and_return(4) + allow(linter).to receive(:metadata_entry).and_return(fake_metadata) + allow(linter).to receive(:locale).and_return('pl_PL') + + numbers = linter.numbers_covering_all_plurals + + expect(numbers).to contain_exactly(0, 1, 2) + end end describe '#fill_in_variables' do @@ -380,3 +424,4 @@ describe Gitlab::I18n::PoLinter do end end end +# rubocop:enable Style/AsciiComments diff --git a/spec/lib/gitlab/i18n/translation_entry_spec.rb b/spec/lib/gitlab/i18n/translation_entry_spec.rb index f68bc8feff9..b301e6ea443 100644 --- a/spec/lib/gitlab/i18n/translation_entry_spec.rb +++ b/spec/lib/gitlab/i18n/translation_entry_spec.rb @@ -109,7 +109,7 @@ describe Gitlab::I18n::TranslationEntry do data = { msgid: %w(hello world) } entry = described_class.new(data, 2) - expect(entry.msgid_contains_newlines?).to be_truthy + expect(entry.msgid_has_multiple_lines?).to be_truthy end end @@ -118,7 +118,7 @@ describe Gitlab::I18n::TranslationEntry do data = { msgid_plural: %w(hello world) } entry = described_class.new(data, 2) - expect(entry.plural_id_contains_newlines?).to be_truthy + expect(entry.plural_id_has_multiple_lines?).to be_truthy end end @@ -127,7 +127,7 @@ describe Gitlab::I18n::TranslationEntry do data = { msgstr: %w(hello world) } entry = described_class.new(data, 2) - expect(entry.translations_contain_newlines?).to be_truthy + expect(entry.translations_have_multiple_lines?).to be_truthy end end