glebm / i18n-tasks

Manage translation and localization with static analysis, for Ruby i18n
http://glebm.github.io/i18n-tasks
MIT License
2.07k stars 263 forks source link

Relative keys from root folder not detected #385

Open fpietka opened 3 years ago

fpietka commented 3 years ago

I have multiple keys defined as .my_translation_key in a model for example, but those keys are in the root of my yaml files.

It works in Rails, and get translated correctly, but using i18n-tasks, those keys are detected in both missing my_translation_key and unused .my_translation_keys.

I tried both path and relative_roots in search configuration, with no luck. I would prefer not to have to rewrite those as they are working. Maybe am I missing something here?

davidwessman commented 2 years ago

@fpietka Can you give an example of a YAML-file with the translations and a small model where you call it?

wollistik commented 2 years ago

@davidwessman I think I have a similar issue: ViewComponents (https://viewcomponent.org) is a framework from Github for building reusable view components for use with Rails. One core concept is that code (a simple Ruby class) and view template (ERB file) reside side by side in the same directory. So the files for a MyComponent class would result in

app/components/my_component.rb
app/components/my_component.html.erb

With the latest release they included a translation feature, which will lookup relative translation keys in a translation file side to side with the other component files. So now it looks like this:

app/components/my_component.rb
app/components/my_component.html.erb
app/components/my_component{.locale}.yml

The key point is, that in this translation files there are no other root keys except the language. The component class/path is omitted, so everything is under root:

# Content of app/components/my_component.yml
en:
  some_key: ...

and the view view I can use

<%= t('.some_key') %>

You can find the documentation of this feature here: https://viewcomponent.org/guide/translations.html

When configuring i18n-tasks to also read the component translations files and check the usages this will end up all component keys are at root level. The health tasks would then report an unused key some_key and a missing key for my_component.some_key.

Since ViewComponent grows in popularity and there were already related issues (see #452 ) this should be proper supported. I think a new configuration option to infer additional keys from file/path would solve the issue. When this option is set for e.g. app/component every encountered translation hash would be enriched with the path and the hash will result in

{
  en: {
           my_component: { some_key: some_value }
        }
}

This must be reverted of course when the files get written.

What do you think?

davidwessman commented 2 years ago

Thank you for great summary and feedback @wollistik I think this relates to the feedback from palkan in https://github.com/github/view_component/issues/892, e.g. that it works poorly with translation services and similar.

I cannot see how i18n-tasks can support reading translations with custom roots from app/components in a simple way, it will be a large change.

@glebm What do you think about native support for ViewComponent?

E.g. finding:

# app/components/event_component.en.yml
en:
  key: Special translation

translating this to en.event_component.key or even en.event_component.key when reading all translations and then write it back to correct location after normalize.

glebm commented 2 years ago

If we can't find a simple way to make this configurable, a plugin/special support for ViewComponent makes sense

One issue I can foresee is that the keys don't seem to share a common prefix (e.g. components.), so routing new keys to the correct files automatically in a general way may be tricky. So pattern_router won't work well but conservative_router should.

JochenLutz commented 3 weeks ago

[…] translating this to en.event_component.key or even en.event_component.key when reading all translations and then write it back to correct location after normalize.

That translation to fully prefixed keys (en.event_component.key) is what ViewComponent does internally. If you use an undefined translation, the popup in the browser shows exactly this key as missing (at least in dev mode).

I have a working prototype that implements that (code at the end). Right now, it exists directly in our code base. It would need some cleanup like not hardcoded paths, but then it should be usable.

The other issue I identified is that ViewComponent supports sidecar directories. With that, for app/components/my_component.rb you have app/components/my_component/my_component.html.erb. See ViewComponent's documentation for an illustrated example. ViewComponent still maps that to en.my_component.key. If you add the translation in normal locale files as en.my_component.my_component.key, it is not used.

As a workaround I monkey-patched I18n::Tasks::Scanners::ErbAstScanner#prefix to ignore this duplicate last path segment. I think a clean solution would be to add a config key similar to relative_exclude_method_name_paths and add that functionality in ErbAstScanner. @glebm would you accept a ticket/PR for that?

For FilesystemWithComponents: what is your suggestion for that? Is that something you would add (with configurability added) into i18n-tasks or should that remain something separate?

module I18nTasks
  class FilesystemWithComponents < ::I18n::Tasks::Data::FileSystemBase
    register_adapter :yaml, '*.yml', ::I18n::Tasks::Data::Adapter::YamlAdapter
    register_adapter :json, '*.json', ::I18n::Tasks::Data::Adapter::JsonAdapter

    def load_file(path)
      result = super

      return result unless component_locale_data?(path)

      locale    = result.keys.first
      key_parts = [locale, *component_keys(path)]

      key_parts.reverse.inject(result[locale]) { |akk, key| { key => akk } }
    end

    def normalized?(path, tree)
      if component_locale_data?(path)
        new_tree = tree.dup

        new_tree.list.first.children = component_keys(path).inject(new_tree.list.first) { |akk, el| akk[el] }.children

        super(path, new_tree)
      else
        super
      end
    end

    private

    def component_locale_data?(path)
      path.start_with?('app/components')
    end

    def component_keys(path)
      File.dirname(path).split('/')[2..]
    end
  end
end

module I18n
  module Tasks
    module Scanners
      class ErbAstScanner
        def prefix(normalized_path, calling_method: nil)
          file_basename = File.basename(normalized_path).gsub(/(\.\w+)?\.erb$/, '')

          if File.dirname(normalized_path).end_with?("/#{file_basename}")
            result = super

            result.gsub(/\.#{file_basename}$/, '')
          else
            super
          end
        end
      end
    end
  end
end
glebm commented 3 weeks ago

I'll defer to @davidwessman who is the author of ErbAstScanner.

I think if the changes are minimal then it might be to have it here, otherwise a separate gem might be better (there are other gems already that add additional functionality to i18n-tasks, e.g. i18n-tasks-csv, and other scanners, such as this one for Vue, the "it" gem, and for Angular).

davidwessman commented 3 weeks ago

@JochenLutz Seems like ViewComponents are quite different to the relative_roots implementation that works for views and controllers :/.

I think I am open to a PR, but I wonder how we can somehow collect all the ViewComponent behaviour into the same classes. Spreading it out through settings, scanners and matchers makes it harder to maintain.

Could we add a scanner that we recommend using for app/components/**/*.erb and another for app/components/**/*.rb? And when we see the code we might decide if it should be a separate gem or not.

Sorry for a rambling answer, but would be easier with a PR, even if it is just a proof-of-concept

JochenLutz commented 1 week ago

Seems like ViewComponents are quite different to the relative_roots implementation that works for views and controllers :/.

I think the main difference for ViewComponents is that they always take the scope for relative keys based only on the components fully qualified name. Apart from that (I think) everything related to the view and the Ruby part of the ViewComponent works normally.

Could we add a scanner that we recommend using for app/components/**/*.erb and another for app/components/**/*.rb?

How would that be done? I tried that, but I did not find a way to stop ErbAstScanner from scanning app/components/**/*.erb — except obviously removing app/components from relative_roots. That would then require to manually add an instance of RubyAstScanner for that directory. Or is there another way?

I have a branch with a patch for ErbAstScanner in my fork. I hope that helps to clarify things. If you want, I can turn that into a PR.

glebm commented 1 week ago

How would that be done? I tried that, but I did not find a way to stop ErbAstScanner from scanning app/components/*/.erb

All scanners support an exclude: argument:

https://github.com/glebm/i18n-tasks/blob/d0453929603379bea15577065fecfc79a045290e/lib/i18n/tasks/scanners/files/file_finder.rb#L10-L16