DEV: Check English locale for errors in CI

Moves the most important checks into a linter. It gets executed by Lefthook as well as the docker rake task and Github actions. Doing those checks in rspec takes too long and it produces errors when the discourse:test Docker image contains old, invalid locale files.

.github/workflows/ci.yml

@@ -6,7 +6,7 @@ on:
       - master
   pull_request:
     branches-ignore:
-      - 'tests-passed'
+      - "tests-passed"
 
 jobs:
   build:
@@ -28,12 +28,12 @@ jobs:
       fail-fast: false
 
       matrix:
-        build_types: [ 'BACKEND', 'FRONTEND', 'LINT' ]
-        target: [ 'PLUGINS', 'CORE' ]
-        os: [ ubuntu-latest ]
-        ruby: [ '2.6' ]
-        postgres: [ '10' ]
-        redis: [ '4.x' ]
+        build_types: ["BACKEND", "FRONTEND", "LINT"]
+        target: ["PLUGINS", "CORE"]
+        os: [ubuntu-latest]
+        ruby: ["2.6"]
+        postgres: ["10"]
+        redis: ["4.x"]
 
     services:
       postgres:
@@ -77,7 +77,7 @@ jobs:
         uses: actions/setup-ruby@v1
         with:
           ruby-version: ${{ matrix.ruby }}
-          architecture: 'x64'
+          architecture: "x64"
 
       - name: Setup bundler
         run: |
@@ -145,6 +145,14 @@ jobs:
           yarn prettier -v
           yarn prettier --list-different "app/assets/stylesheets/**/*.scss" "app/assets/javascripts/**/*.js" "app/assets/javascripts/**/*.es6" "test/javascripts/**/*.es6" "plugins/**/*.scss" "plugins/**/*.es6"
 
+      - name: Core English locale
+        if: env.BUILD_TYPE == 'LINT' && env.TARGET == 'CORE'
+        run: bundle exec ruby script/i18n_lint.rb "config/**/locales/{client,server}.en.yml"
+
+      - name: Plugin English locale
+        if: env.BUILD_TYPE == 'LINT' && env.TARGET == 'PLUGINS'
+        run: bundle exec ruby script/i18n_lint.rb "plugins/**/locales/{client,server}.en.yml"
+
       - name: Core RSpec
         if: env.BUILD_TYPE == 'BACKEND' && env.TARGET == 'CORE'
         run: |
@@ -167,5 +175,5 @@ jobs:
 
       - name: Plugin QUnit # Tests core plugins in TARGET=CORE, and all plugins in TARGET=PLUGINS
         if: env.BUILD_TYPE == 'FRONTEND'
-        run: bundle exec rake plugin:qunit
+        run: bundle exec rake plugin:qunit['*','1200000']
         timeout-minutes: 30

lefthook.yml

@@ -23,6 +23,9 @@ pre-commit:
       # database.yml is an erb file not a yaml file
       exclude: "database.yml"
       run: bundle exec yaml-lint {staged_files}
+    i18n-lint:
+      glob: "**/{client,server}.en.yml"
+      run: bundle exec ruby script/i18n_lint.rb {staged_files}
 
 commands: &commands
   bundle-install:
@@ -58,8 +61,11 @@ lints:
     eslint-test:
       run: yarn eslint --ext .es6 test/javascripts
     eslint-plugins-assets:
-      run: yarn eslint --ext .es6 plugins/**/assets/javascripts
+      run: yarn eslint --global I18n --ext .es6 plugins/**/assets/javascripts
     eslint-plugins-test:
-      run: yarn eslint --ext .es6 plugins/**/test/javascripts
+      run: yarn eslint --global I18n --ext .es6 plugins/**/test/javascripts
     eslint-assets-tests:
       run: yarn eslint app/assets/javascripts test/javascripts
+    i18n-lint:
+      glob: "**/{client,server}.en.yml"
+      run: bundle exec ruby script/i18n_lint.rb {all_files}

lib/tasks/docker.rake

@@ -66,6 +66,7 @@ task 'docker:test' do
 
       if ENV["SINGLE_PLUGIN"]
         @good &&= run_or_fail("bundle exec rubocop --parallel plugins/#{ENV["SINGLE_PLUGIN"]}")
+        @good &&= run_or_fail("bundle exec ruby script/i18n_lint.rb plugins/#{ENV["SINGLE_PLUGIN"]}/config/locales/{client,server}.en.yml")
         @good &&= run_or_fail("yarn eslint --global I18n --ext .es6 plugins/#{ENV['SINGLE_PLUGIN']}")
 
         puts "Listing prettier offenses in #{ENV['SINGLE_PLUGIN']}:"
@@ -78,6 +79,9 @@ task 'docker:test' do
         # TODO: remove --global I18n once plugins can be updated
         @good &&= run_or_fail("yarn eslint --global I18n --ext .es6 plugins") unless ENV["SKIP_PLUGINS"]
 
+        @good &&= run_or_fail('bundle exec ruby script/i18n_lint.rb "config/locales/{client,server}.en.yml"') unless ENV["SKIP_CORE"]
+        @good &&= run_or_fail('bundle exec ruby script/i18n_lint.rb "plugins/**/locales/{client,server}.en.yml"') unless ENV["SKIP_PLUGINS"]
+
         unless ENV["SKIP_CORE"]
           puts "Listing prettier offenses in core:"
           @good &&= run_or_fail('yarn prettier --list-different "app/assets/stylesheets/**/*.scss" "app/assets/javascripts/**/*.es6" "test/javascripts/**/*.es6"')

script/i18n_lint.rb

@@ -0,0 +1,129 @@
+# frozen_string_literal: true
+
+require 'colored2'
+require 'psych'
+
+class I18nLinter
+  def initialize(filenames_or_patterns)
+    @filenames = filenames_or_patterns.map { |fp| Dir[fp] }.flatten
+    @errors = {}
+  end
+
+  def run
+    has_errors = false
+
+    @filenames.each do |filename|
+      validator = LocaleFileValidator.new(filename)
+
+      if validator.has_errors?
+        validator.print_errors
+        has_errors = true
+      end
+    end
+
+    exit 1 if has_errors
+  end
+end
+
+class LocaleFileValidator
+  ERROR_MESSAGES = {
+    invalid_relative_links: "The following keys have relative links, but do not start with %{base_url} or %{base_path}:",
+    invalid_relative_image_sources: "The following keys have relative image sources, but do not start with %{base_url} or %{base_path}:",
+    invalid_interpolation_key_format: "The following keys use {{key}} instead of %{key} for interpolation keys:",
+    wrong_pluralization_keys: "Pluralized strings must have only the sub-keys 'one' and 'other'.\nThe following keys have missing or additional keys:",
+    invald_one_keys: "The following keys contain the number 1 instead of the interpolation key %{count}:"
+  }
+
+  PLURALIZATION_KEYS = ['zero', 'one', 'two', 'few', 'many', 'other']
+  ENGLISH_KEYS = ['one', 'other']
+
+  def initialize(filename)
+    @filename = filename
+    @errors = {}
+  end
+
+  def has_errors?
+    yaml = Psych.safe_load(File.read(@filename), aliases: true)
+    yaml = yaml[yaml.keys.first]
+
+    validate_pluralizations(yaml)
+    validate_content(yaml)
+
+    @errors.any? { |_, value| value.any? }
+  end
+
+  def print_errors
+    puts "", "Errors in #{@filename}".red
+
+    @errors.each do |type, keys|
+      next if keys.empty?
+
+      ERROR_MESSAGES[type].split("\n").each { |msg| puts "  #{msg}" }
+      keys.each { |key| puts "    * #{key}" }
+    end
+  end
+
+  private
+
+  def each_translation(hash, parent_key = '', &block)
+    hash.each do |key, value|
+      current_key = parent_key.empty? ? key : "#{parent_key}.#{key}"
+
+      if Hash === value
+        each_translation(value, current_key, &block)
+      else
+        yield(current_key, value.to_s)
+      end
+    end
+  end
+
+  def validate_content(yaml)
+    @errors[:invalid_relative_links] = []
+    @errors[:invalid_relative_image_sources] = []
+    @errors[:invalid_interpolation_key_format] = []
+
+    each_translation(yaml) do |key, value|
+      if value.match?(/href\s*=\s*["']\/[^\/]|\]\(\/[^\/]/i)
+        @errors[:invalid_relative_links] << key
+      end
+
+      if value.match?(/src\s*=\s*["']\/[^\/]/i)
+        @errors[:invalid_relative_image_sources] << key
+      end
+
+      if value.match?(/{{.+?}}/) && !key.end_with?("_MF")
+        @errors[:invalid_interpolation_key_format] << key
+      end
+    end
+  end
+
+  def each_pluralization(hash, parent_key = '', &block)
+    hash.each do |key, value|
+      if Hash === value
+        current_key = parent_key.empty? ? key : "#{parent_key}.#{key}"
+        each_pluralization(value, current_key, &block)
+      elsif PLURALIZATION_KEYS.include? key
+        yield(parent_key, hash)
+      end
+    end
+  end
+
+  def validate_pluralizations(yaml)
+    @errors[:wrong_pluralization_keys] = []
+    @errors[:invald_one_keys] = []
+
+    each_pluralization(yaml) do |key, hash|
+      # ignore errors from some ActiveRecord messages
+      next if key.include?("messages.restrict_dependent_destroy")
+
+      @errors[:wrong_pluralization_keys] << key if hash.keys.sort != ENGLISH_KEYS
+
+      one_value = hash['one']
+      if one_value && one_value.include?('1') && !one_value.match?(/%{count}|{{count}}/)
+        @errors[:invald_one_keys] << key
+      end
+    end
+  end
+end
+
+I18nLinter.new(ARGV).run

spec/integrity/i18n_spec.rb

@@ -7,22 +7,6 @@ def extract_locale(path)
   path[/\.([^.]{2,})\.yml$/, 1]
 end
 
-PLURALIZATION_KEYS ||= ['zero', 'one', 'two', 'few', 'many', 'other']
-ENGLISH_KEYS ||= ['one', 'other']
-
-def find_pluralizations(hash, parent_key = '', pluralizations = Hash.new)
-  hash.each do |key, value|
-    if Hash === value
-      current_key = parent_key.blank? ? key : "#{parent_key}.#{key}"
-      find_pluralizations(value, current_key, pluralizations)
-    elsif PLURALIZATION_KEYS.include? key
-      pluralizations[parent_key] = hash
-    end
-  end
-
-  pluralizations
-end
-
 def is_yaml_compatible?(english, translated)
   english.each do |k, v|
     if translated.has_key?(k)
@@ -39,18 +23,6 @@ def is_yaml_compatible?(english, translated)
   true
 end
 
-def each_translation(hash, parent_key = '', &block)
-  hash.each do |key, value|
-    current_key = parent_key.blank? ? key : "#{parent_key}.#{key}"
-
-    if Hash === value
-      each_translation(value, current_key, &block)
-    else
-      yield(current_key, value.to_s)
-    end
-  end
-end
-
 describe "i18n integrity checks" do
 
   it 'has an i18n key for each Trust Levels' do
@@ -107,71 +79,6 @@ describe "i18n integrity checks" do
         english_duplicates = DuplicateKeyFinder.new.find_duplicates(english_path)
         expect(english_duplicates).to be_empty
       end
-
-      context "pluralizations" do
-        wrong_keys = []
-        invald_one_keys = []
-
-        find_pluralizations(english_yaml).each do |key, hash|
-          next if key["messages.restrict_dependent_destroy"]
-
-          wrong_keys << key if hash.keys.sort != ENGLISH_KEYS
-
-          if one_value = hash['one']
-            invald_one_keys << key if one_value.include?('1') && !one_value.match?(/%{count}|{{count}}/)
-          end
-        end
-
-        it "has valid pluralizations keys" do
-          keys = wrong_keys.join("\n")
-          expect(wrong_keys).to be_empty, <<~MSG
-            Pluralized strings must have only the sub-keys 'one' and 'other'.
-            The following keys have missing or additional keys:\n\n#{keys}
-          MSG
-        end
-
-        it "should use %{count} instead of 1 in 'one' keys" do
-          keys = invald_one_keys.join(".one\n")
-          expect(invald_one_keys).to be_empty, <<~MSG
-            The following keys contain the number 1 instead of the interpolation key %{count}:\n\n#{keys}
-          MSG
-        end
-      end
-
-      context "valid translations" do
-        invalid_relative_links = {}
-        invalid_relative_image_sources = {}
-        invalid_interpolation_key_format = {}
-
-        each_translation(english_yaml) do |key, value|
-          if value.match?(/href\s*=\s*["']\/[^\/]|\]\(\/[^\/]/i)
-            invalid_relative_links[key] = value
-          end
-
-          if value.match?(/src\s*=\s*["']\/[^\/]/i)
-            invalid_relative_image_sources[key] = value
-          end
-
-          if value.match?(/\{\{.+?}}/)
-            invalid_interpolation_key_format[key] = value
-          end
-        end
-
-        it "uses %{base_url} or %{base_path} for relative links" do
-          keys = invalid_relative_links.keys.join("\n")
-          expect(invalid_relative_links).to be_empty, "The following keys have relative links, but do not start with %{base_url} or %{base_path}:\n\n#{keys}"
-        end
-
-        it "uses %{base_url} or %{base_path} for relative image src" do
-          keys = invalid_relative_image_sources.keys.join("\n")
-          expect(invalid_relative_image_sources).to be_empty, "The following keys have relative image sources, but do not start with %{base_url} or %{base_path}:\n\n#{keys}"
-        end
-
-        skip "uses the %{key} as interpolation key format" do
-          keys = invalid_interpolation_key_format.keys.join("\n")
-          expect(invalid_interpolation_key_format).to be_empty, "The following keys use {{key}} instead of %{key} for interpolation keys:\n\n#{keys}"
-        end
-      end
     end
 
     Dir[english_path.sub(".en.yml", ".*.yml")].each do |path|