Add I18n syntax error translation #241

sirupsen · 2013-08-15T03:32:47Z

Simple possible implementation of I18n support for Liquid errors, as requested in #18.

@fw42 please take a look.

fw42 · 2013-08-15T11:09:14Z

test/test_helper.rb

-require File.join(File.dirname(__FILE__), '..', 'lib', 'liquid')
+
+$:.unshift(File.join(File.expand_path(File.dirname(__FILE__)), '..', 'lib'))
+require 'liquid.rb'


Why this change?

Doesn't handle running single test files nicely otherwise.

Hm, didn't have problems with that before. What do you mean exactly?

liquid (i18n-error) Σ ruby -Itest test/liquid/i18n_test.rb /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require': cannot load such file -- liquid/version (LoadError) from /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require' from /Users/sirup/Dropbox/Code/liquid/lib/liquid.rb:48:in `<top (required)>' from /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require' from /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require' from /Users/sirup/Dropbox/Code/liquid/test/test_helper.rb:13:in `<top (required)>' from /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require' from /opt/boxen/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require' from test/liquid/i18n_test.rb:1:in `<main>'

Do you run ruby -Itest -Ilib test/liquid/i18n_test.rb? I rarely see that.

👍 i had the same problem before. I would, however, have this change in a isolate commit.

fw42 · 2013-08-15T11:23:55Z

Hm, haven't taken a very close look, but considering we only have like 5 error messages at most, this seems very overkill. Also, I don't want to call it "I18n", that's a bit confusing.

fw42 · 2013-08-15T11:25:22Z

@phoet, thoughts?

fw42 · 2013-08-15T11:25:48Z

lib/liquid/i18n.rb

@@ -0,0 +1,46 @@
+require 'yaml'
+require 'delegate'


What's this for?

Previous attempt, I forgot to remove it.

fw42 · 2013-08-15T11:30:17Z

Hm, ok, on second thought, maybe it's not overkill. Seems nice actually. @arthurnn, @dylanahsmith, any comments so far?

fw42 · 2013-08-15T11:32:44Z

lib/liquid/template.rb

@@ -41,7 +41,9 @@ def register_filter(mod)
      end

      # creates a new <tt>Template</tt> object from liquid source code
-      def parse(source)
+      def parse(source, options = {:locale => "lib/liquid/locales/en.yml"})


I think this is not the right place to define default options. There is two ways to parse a template: Template.parse(...) and Template.new.parse(...). You only catch one of them. Also, your I18n.global thing doesn't seem to be thread-safe (two threads using different languages). Instead of using global variables, why not store this stuff in the Liquid context?

Yeah, it's not thread-safe. It hit me when I went to bed I could store the I18n object in the Liquid context registers to access it anywhere, but haven't actually implemented it yet. I think this is what makes it not overkill, that you can have these objects around. It's a pretty simple class, too.

Also, will this relative path work once this thing is packaged as a gem (and the application itself doesn't provide a locale file)?

Like in the test you wanna make it absolute.

sirupsen · 2013-08-15T11:56:18Z

Since I18n is self-referential, it can stack overflow if the error message doesn't exist in the database. There are three solutions:

We don't care to translate errors in I18n.
Add some logic that handles this case and presents a better error than a stack overflow.
We don't really care about this case as people should generally be able to copy the english locale and go from there and never have to worry about this.

fw42 · 2013-08-15T14:28:50Z

lib/liquid/i18n.rb

+
+    private
+    def interpolate(name, vars)
+      name.gsub(/:(\w+)/) do


I feel like this might match too much. Maybe we should add the possibility to escape :? Like /[^\]:(\w+)/ maybe?

edit: Good point!

Just for the case that someone wants to include colons in their error messages. Dunno. Not a big deal.

sirupsen · 2013-08-16T17:10:29Z

@fw42 What should be done? Thoughts on whether the logic in Template#initialize should be moved?

fw42 · 2013-08-16T17:11:37Z

Wait for one of the three people I asked for comments to actually say something :P

fw42 · 2013-08-19T20:02:52Z

Guys, can one of you please take a look? @arthurnn, @dylanahsmith, @phoet, @trishume

arthurnn · 2013-08-19T20:11:04Z

lib/liquid/template.rb

@@ -101,7 +102,7 @@ def render(*args)
      when nil
        Context.new(assigns, instance_assigns, registers, @rethrow_errors, @resource_limits)
      else
-        raise ArgumentError, "Expect Hash or Liquid::Context as parameter"
+        raise ArgumentError, registers[:locale].translate("errors.template.argument_hash_or_context")


why not using the .t method in here? 🐹

I don't think this message would be expected to ever how up in rendered liquid output, so do we really need to translate this?

Do we only need to translate syntax errors? I don't know Liquid well in terms of what could be useful to translate besides that. But yes, agreed. 👍 Shouldn't use this everywhere, just where it's needed to not clutter the locale file too much.

There are a handful of errors other than SyntaxError. Try grep raise -R lib in the repository.

Sure, but which of those do we actually need to translate? Should they all be translated?

fw42 · 2013-08-19T20:12:21Z

I'm still not sure about calling this I18n. It's confusing.

trishume · 2013-08-19T20:17:01Z

lib/liquid/locales/en.yml

+      unknown_translation: "Translation for :name does not exist in locale"
+      undefined_interpolation: "Undefined key :key for interpolation in translation :name"
+    template:
+      argument_hash_or_context: "Expect Hash or Liquid::Context as parameter"


Doesn't the :Context in this get treated as an interpolation?

Maybe a different interpolation character should be chosen, colons are common in plain english and code.

It might be clearer if we use /%{\w+}/ syntax for interpolation.

That's true. There is support for escaping, but it feels really awkward here. I support your idea @dylanahsmith, @fw42 any strong feelings about :symbol?

Nope, no strong feelings

I like '%' as the escape character as well, for consistency with C printf. Or maybe for the recursive lulz the strings could be interpreted using Liquid.

trishume · 2013-08-19T20:24:50Z

Looks pretty good. I agree about not naming it I18n though, maybe 'Internationalization'?

arthurnn · 2013-08-19T20:25:37Z

I like the I18n name , as long as we always use it under the Liquid:: namespace .

fw42 · 2013-08-19T23:48:55Z

lib/liquid/i18n.rb

+    private
+    def interpolate(name, vars)
+      name.gsub(/([^\\]):(\w+)/) {
+        raise TranslationError, translate("errors.i18n.undefined_interpolation", :key => $1, :name => name) unless vars[$2.to_sym]


Instead of raising, maybe just don't interpolate?

Would you ever want to put the interpolation syntax if the key is not passed? Shouldn't this act as much like Ruby as possible?

Hm yeah I guess you are right

sirupsen · 2013-08-20T02:13:14Z

What's do you guys not like about I18n @trishume @fw42 as long as it is namespaced properly everywhere? The reason why people call things I18n and not Internationalization is because that word is so absurdly long.

fw42 · 2013-08-20T02:29:58Z

I was under the impression that most people associate the term "I18n" with one specific implementation of Internationalization (the one that comes with Rails). I didn't know it's just a shorter synonym. Feel free to use it then.

arthurnn · 2013-08-20T02:31:01Z

It is indeed just a short term, on my old times of java development, we used to use I18n for classes and all that. http://struts.apache.org/release/1.3.x/apidocs/org/apache/struts/tiles/xmlDefinition/I18nFactorySet.html

phoet · 2013-08-20T07:11:24Z

@fw42 sorry, i am quite bussy right now and on vacation from thursday on. will have no time to review!

sirupsen · 2013-08-21T17:19:47Z

@arthurnn @trishume @fw42 I removed the option passing to parse and changed the interpolation syntax.

arthurnn · 2013-08-21T17:29:21Z

Really like the lib/liquid/template.rb now.. 💚 ❤️ 🍍 ❤️ 💚 .

Probably before merge we would want to squash those back and forth in one commit, but besides that I like it.
👍 on my side.

sirupsen · 2013-08-21T17:37:14Z

Also need to actually add the translations. :-) Still don't know whether every exception should be translated, or just syntax errors. See thread above.

trishume · 2013-08-21T17:57:25Z

This looks great, I like the interpolation syntax. In terms of translations I think it should be every error that can be produced by problems with a template, which means any error that a user rather than a developer will see.

That means syntax errors as well as some other errors like 'missing filter'.

sirupsen · 2013-08-29T17:23:00Z

I'll wait with this until the new parser is merged in.

fw42 · 2013-08-29T17:59:32Z

@sirupsen, @trishume, lets try to get both this and the new parser shipped by tomorrow.

trishume · 2013-08-30T13:10:08Z

@fw42 Agreed! Now that this PR encompasses both the new parser and fancy errors we can just merge this one and close the other.

fw42 · 2013-08-30T13:16:53Z

I would rather have two separate merges (first the parser branch, then this one). ⛵ 💨

sirupsen · 2013-08-30T13:18:53Z

@fw42 agreed, that's cleaner

fw42 · 2013-08-30T14:32:32Z

lib/liquid/lexer.rb

    end

    def tokenize
      @output = []

-      loop do
+      while !@ss.eos?


What's this? Why is this PR changing lexer code?

That's weird.. :s Bad rebase? @trishume any ideas? You changed this recently?

I saw that too, I have no idea why. If you look at lexer.rb in master right now it has the changes that this PR is supposedly adding.

Maybe just a GitHub UI fuckup? Can you check manually before merging?

You could do the merge on your computer instead of in the Github UI I guess...

fw42 · 2013-08-30T14:42:41Z

Couple of small comments, other than that, 👍

Add I18n syntax error translation

fw42 reviewed Aug 15, 2013
View reviewed changes

arthurnn reviewed Aug 19, 2013
View reviewed changes

trishume reviewed Aug 19, 2013
View reviewed changes

fw42 reviewed Aug 19, 2013
View reviewed changes

fw42 reviewed Aug 30, 2013
View reviewed changes

sirupsen added 13 commits August 30, 2013 12:31

Add sketch of I18n error translation

f37a984

Change to absolute path in localization test

0dac6fe

Remove delegate require from localization

e7bcf04

Add fixture helper

0a2f213

Add locale to context registers

40fba9e

Add escaping of symbols

0343f6d

Move localization option to register

5ee4f96

Change interpolation syntax to %{key}

df5980f

Move I18n to options

29cdabc

Localize errors in Liquid

072c12d

Fix use of 1.9 hash syntax

fc8c45e

Rename outdated test

380828f

Remove superplus translations

ad184fb

sirupsen added a commit that referenced this pull request Aug 30, 2013

Merge pull request #241 from Shopify/i18n-error

136b676

Add I18n syntax error translation

sirupsen merged commit 136b676 into master Aug 30, 2013

fw42 mentioned this pull request Aug 30, 2013

Translation of error messages #18

Closed

Add I18n syntax error translation #241

Add I18n syntax error translation #241

Conversation

sirupsen commented Aug 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fw42 commented Aug 15, 2013

fw42 commented Aug 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fw42 commented Aug 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen commented Aug 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen commented Aug 16, 2013

fw42 commented Aug 16, 2013

fw42 commented Aug 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fw42 commented Aug 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trishume commented Aug 19, 2013

arthurnn commented Aug 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen commented Aug 20, 2013

fw42 commented Aug 20, 2013

arthurnn commented Aug 20, 2013

phoet commented Aug 20, 2013

sirupsen commented Aug 21, 2013

arthurnn commented Aug 21, 2013

sirupsen commented Aug 21, 2013

trishume commented Aug 21, 2013

sirupsen commented Aug 29, 2013

fw42 commented Aug 29, 2013

trishume commented Aug 30, 2013

fw42 commented Aug 30, 2013

sirupsen commented Aug 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fw42 commented Aug 30, 2013