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 --config to specify config file location #394

Merged
merged 1 commit into from
Oct 28, 2021
Merged

Conversation

krtschmr
Copy link
Contributor

@krtschmr krtschmr commented Oct 27, 2021

We are running I18n checks for our backend translations and also for for frontend JS files. We do have a seperated config file for those and had a rspec test like this

 context 'JS' do
    before do
      @config_files_was = I18n::Tasks::Configuration::CONFIG_FILES
      I18n::Tasks::Configuration::CONFIG_FILES = %w[
        config/i18n-js-tasks.yml
      ].freeze
    end

    after do
      I18n::Tasks::Configuration::CONFIG_FILES = @config_files_was
    end

    it 'files are normalized' do
      non_normalized = i18n.non_normalized_paths
      error_message = "The following files need to be normalized:\n" \
                      "#{non_normalized.map { |path| "  #{path}" }.join("\n")}\n" \
                      'Please run `rake i18n_js_tasks:run["normalize"]` to fix'
      expect(non_normalized).to be_empty, error_message
    end
  end

Since our test suite is big and takes a while (~1 hour) on CI , we don't want to waste resources anymore, only to realize after the test run, that a translation key is missing. Quite inefficient and unnecessary. We looked for a way to also run the config file for our JS checks via shell command, but that wasn't possible.

We found a way of actually adding the config file to the health check. Since this gem actually relies heavily on the config file before it does anything else, i had to inject the config file loadup right at the beginning.

Most likely this PR won't be accepted and if there are just minor things that are missing i'm happy to take on them, but i fear that it's a bigger architecture problem. Maybe there are better ways to actually implement this which i didn't find on my first try.

If this won't get merged, maybe somebody else might stumble across this and it's helpful for him, or at least this PR is acting as an inspiration for future progress.

We can call this now with i18n-tasks health (default config location) and ì18n-tasks health --config config/i18n-js-yml (for our JS tasks)

@@ -2,6 +2,6 @@

module I18n
module Tasks
VERSION = '0.9.34'
VERSION = '0.9.35'
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert the change to this file, version bumps go into separate commits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ done

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.

2 participants