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 post_install_message about change of fallbacks #442

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

kyamaguchi
Copy link
Contributor

@kyamaguchi kyamaguchi commented Dec 5, 2018

We experienced problem with our projects by the change of fallbacks behavior.
We wanted to know the breaking change.

I want any users who will update i18n to know the change to avoid troubles.
I don't mind the detail(content) of post_install_message.
I borrowed an idea from the other popular gem haml.
https://github.com/haml/haml/blob/f699ca90cf5ce09f71035aebe8c3b6d67401ebfa/haml.gemspec#L37

Main issue:
#415

Changes in Rails 6:
rails/rails#34383
rails/rails#33574


Not all users are affected by the fallbacks issue. (Affected users are much less.)
So post_install_message could be too much because it will be displayed to all users.

The better alternative may be displaying warning message.

lib/i18n/locale/fallbacks.rb

@@ -56,7 +56,7 @@ module I18n
       def initialize(*mappings)
         @map = {}
         map(mappings.pop) if mappings.last.is_a?(Hash)
-        self.defaults = mappings.empty? ? [] : mappings
+        self.defaults = mappings.empty? ? empty_with_warning : mappings
       end

       def defaults=(defaults)
@@ -91,6 +91,13 @@ module I18n
         result.push(*defaults) if include_defaults
         result.uniq.compact
       end
+
+      private
+
+      def empty_with_warning
+        puts "Warning message"
+        []
+      end
     end

Rails introduced message but it doesn't help the majority of current Rails users who use Rails (< 6.0).
https://github.com/rails/rails/pull/33574/files

i18n.gemspec Outdated
If not, fallbacks will be broken in your app by I18n 1.1.x.

For more info see:
https://github.com/svenfuchs/i18n/releases
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should point to the exact release that broke it.

@radar
Copy link
Collaborator

radar commented Dec 7, 2018

Thank you @kyamaguchi. I agree that we should add a post install message. I've got one small suggestion and then I think this PR is good to go.

@kyamaguchi
Copy link
Contributor Author

@radar

I've got one small suggestion

Fixed

@radar
Copy link
Collaborator

radar commented Dec 8, 2018

Thank you! I’ll merge this when I am back at work on Monday.

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