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

Introduce support for deprecating translation keys #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aldesantis
Copy link

@aldesantis aldesantis commented Sep 25, 2019

This was born out of the discussion on solidusio/solidus_auth_devise#173. It was originally started by @kennyadsl and completed by me.

The rationale and logic are explained in the commit message for future reference.

Please note that it's very experimental and we understand it may be too complex and/or opinionated to be introduced into I18n directly. We're happy to accept any change requests or even to make it a separate gem if you feel like I18n is not a good place for it.

It's still a bit rough at the edges. If this sparks enough interest, we'd like to:

  • Move the translate patches to Deprecator. This would be much cleaner, but it has some performance drawbacks, mainly that we'd need to call lookup in Deprecator just to check if the keys are defined, but then still delegate to super for the rest of the logic, and super would call lookup again.
  • Improve the deprecation warnings by using a proper logger and/or a more configurable approach.
  • Add API and wiki documentation.
  • Maybe allow people to turn the behavior on/off entirely? Not everyone may like the idea of their translation keys being overridden automatically. However, this might create more problems than it solves, so we're not sure at this point.

Hopefully, the current state of it is enough for you to get an idea!

@aldesantis aldesantis force-pushed the deprecated-keys branch 2 times, most recently from 981b12d to c229b54 Compare September 25, 2019 12:20
Adds an `I18n::deprecate` API that allows users to mark a key as
deprecated and superseded by another.

`I18n::t` will then use the following logic:

- If the key supersedes a deprecated one, and the old one is still
  defined, use the old one instead. If the old one is not defined
  anymore, use the new one.

- If the key is deprecated and the old one is still defined, use
  the old one. If the old one is not defined anymore, use the new
  one.

- For all other keys, regular behavior is maintained.

The reason the old key is always preferred to the new one when
available is that users may have been using the new key for other
purposes, and we don't want to swap it for the old one without
notice. Instead, users have to manually go in and delete the old
key for the new one to start being used.
Copy link

@elia elia left a comment

Choose a reason for hiding this comment

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

Just one suggestion on how to print the deprecation 👍

new_key = I18n.config.deprecations[normalized_key]

if new_key
puts "DEPRECATION WARNING: #{normalized_key} is deprecated, use #{new_key} instead"
Copy link

Choose a reason for hiding this comment

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

Consider using warn (printing to stderr) instead of puts, as it is more common for deprecations.
You might also want to use warn(…, uplevel: 1) for rubies that support it, but you need to weight the checks on ruby version vs. the usability improvement of knowing when the error is originated.

@papa-cool
Copy link

Hi

I like the idea to handle deprecation on translations.
But I think we can have a simpler approach. Instead of adding another layer in the inheritance stack, I think we can take benefit of actual I18n functionality.
We can have several backend using I18n::Backend::Chain, and I18n will fallback from I18n::Backend::Deprecator to I18n::Backend::Simple or anything else.

We can do it with this kind of implementation:

module I18n
  module Backend
    class Deprecator

      def store_translations(locale, data, options = EMPTY_HASH)
        ...
      end

      def available_locales
        ...
      end

      def lookup(locale, key, scope = [], options = EMPTY_HASH)
        key = find_the_key_in_the_dedicated_store
        Warning.warn("Deprecated key #{key}") if key
        return nil # Always return nil to get the fallback on I18n::Backend::Simple or other backend in the chain.
      end
    end
  end
end

then I18n.backend = I18n::Backend::Chain.new(I18n::Backend::Deprecator.new, I18n.backend)

This way, we won't change anything in I18n::Backend::Base.
What do you think about it?

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.

None yet

4 participants