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

Backends with Enumerable #71

Merged
merged 13 commits into from Aug 12, 2017

Conversation

Projects
None yet
2 participants
@shioyama
Owner

shioyama commented Aug 11, 2017

Resolves #39.

This is a relatively small change, but an important one. Currently backends don't strictly "know" what translations they have. They only know how to query for a translation in a locale, or write to that locale.

There are many cases where you'd want to know what locales are available independent of the implementing backend, so it makes sense to add a method to get locales.

Originally I was just going to add a locales method which would do this, but then it occurred to me that hey, this is Ruby, so let's make it more flexible by doing it the Ruby way. So instead of defining a locales method, I simply defined an each method on every backend which yields each locale successively to a block.

In Mobility::Backend, I then also added a list method which gets the list of locales from the iterator. I also included Enumerable so that you can now call any enumerable method ont he backend and it apply it to the locales, which you can then build off to do anything you'd really want to do:

post = Post.create(title: "My Title")
backend = post.title_backend
backend.find { |locale| backend.read(locale) == "My Title" }

The only thing I hesitated about slightly was whether the iterator should pass the locale and the translated value. I think in the end though, considering that for some backends it might be expensive to do operations which fetch values, it's better to implement the minimum, and on top of that you can read/write any value with read and write.

In the example above, you can get just a list of locales with post.title_backend.list:

post = Post.create(title: "My Title")
Mobility.with_locale(:ja) { post.title = "タイトル" }
post.title_backend.list

@cbothner @jnylen Have a look and see what you think.

@shioyama shioyama changed the title from Make backends enumerable to Backends with Enumerable Aug 11, 2017

@cbothner

This comment has been minimized.

Show comment
Hide comment
@cbothner

cbothner Aug 11, 2017

This is very exciting. Awesome work! The one thing I’m not sure about is the name of the #list method, which doesn’t seem particularly ruby-like. From Enumerable, one option would be to leave it as #to_a, but that doesn’t seem semantically correct—is the array representation of a backend really a list of locales? I think I would want at least an alias #locales since it’s the most self documenting at the call site.

This is very exciting. Awesome work! The one thing I’m not sure about is the name of the #list method, which doesn’t seem particularly ruby-like. From Enumerable, one option would be to leave it as #to_a, but that doesn’t seem semantically correct—is the array representation of a backend really a list of locales? I think I would want at least an alias #locales since it’s the most self documenting at the call site.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Aug 11, 2017

Owner

The one thing I’m not sure about is the name of the #list method, which doesn’t seem particularly ruby-like.

Good point! I agree to_a doesn't sound quite right either though. I can change the method name to locales, that does sound more natural.

Regarding the appropriateness of iterator methods on the backend instance, I originaly named the backend instance title_translations (etc) rather than title_backend, with the thinking that although actions on the backend will really be actions on translations so this would sound more natural. I decided to use the _backend suffix in the end because I was worried about namespacing conflicts... but looking at this now, title_translations.each certainly sounds more natural than title_backend.each.

It's these very subtle decisions that are the hardest... I suppose for now we should just stick with the _backend suffix, but if anyone has opinions I'd be eager to hear them.

Owner

shioyama commented Aug 11, 2017

The one thing I’m not sure about is the name of the #list method, which doesn’t seem particularly ruby-like.

Good point! I agree to_a doesn't sound quite right either though. I can change the method name to locales, that does sound more natural.

Regarding the appropriateness of iterator methods on the backend instance, I originaly named the backend instance title_translations (etc) rather than title_backend, with the thinking that although actions on the backend will really be actions on translations so this would sound more natural. I decided to use the _backend suffix in the end because I was worried about namespacing conflicts... but looking at this now, title_translations.each certainly sounds more natural than title_backend.each.

It's these very subtle decisions that are the hardest... I suppose for now we should just stick with the _backend suffix, but if anyone has opinions I'd be eager to hear them.

@cbothner

This comment has been minimized.

Show comment
Hide comment
@cbothner

cbothner Aug 11, 2017

It sure would sound better to iterate over title_translations, but I understand why you wanted to be careful about conflicts. Maybe for the next major version—that would be a worthy API redesign in my opinion.

It sure would sound better to iterate over title_translations, but I understand why you wanted to be careful about conflicts. Maybe for the next major version—that would be a worthy API redesign in my opinion.

shioyama added some commits Aug 11, 2017

yield -> yieldparam
[ci skip]
Define each_locale as backend method
We can define the minimum simplest method on each backend which yields
locales only, and then in Mobility::Backend use this to yield locales
and values for that locale together.

Mobility::Backend#locales should return all locales *without* reading
the actual values (since this could potentially be costly for backends),
which means we can't simply use the `to_a` enumerable method here, and
instead need to build up an array iterating through each locale.
@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Aug 12, 2017

Owner

@cbothner After thinking about this for a while, I've settled on the following solution.

First, backends implement each_locale rather than each itself. I think this is clearer since what they are actually yielding is locales, which is the minimum they need to do.

Then in Mobility::Backend, I define the actual each iterator in terms of this each_locale as follows:

def each
  each_locale { |locale| yield Translation.new(self, locale) }
end

Here, Translation is just a struct that takes the backend and the locale, with methods read and write which delegate to read and write on the backend:

Translation = Struct.new(:backend, :locale) do
  %w[read write].each do |accessor|
    define_method accessor do |*args|
      backend.send(accessor, locale, *args)
    end
  end
end

Now if you call any iterator method, the values yielded are these Translation structs, from which you can get the actual value by calling read:

post.title_backend.find { |t| t.read == "foo" }
#=> returns translation with title "foo"

Since write is also defined, you can easily write as well.

Also, locales is simple to define, and reads very natural:

def locales
  map(&:locale)
end

I may be overthinking this a bit, but it feels like this is the right balance. If post.title_backend were renamed to post.title_translations, then this would feel all the more natural.

Owner

shioyama commented Aug 12, 2017

@cbothner After thinking about this for a while, I've settled on the following solution.

First, backends implement each_locale rather than each itself. I think this is clearer since what they are actually yielding is locales, which is the minimum they need to do.

Then in Mobility::Backend, I define the actual each iterator in terms of this each_locale as follows:

def each
  each_locale { |locale| yield Translation.new(self, locale) }
end

Here, Translation is just a struct that takes the backend and the locale, with methods read and write which delegate to read and write on the backend:

Translation = Struct.new(:backend, :locale) do
  %w[read write].each do |accessor|
    define_method accessor do |*args|
      backend.send(accessor, locale, *args)
    end
  end
end

Now if you call any iterator method, the values yielded are these Translation structs, from which you can get the actual value by calling read:

post.title_backend.find { |t| t.read == "foo" }
#=> returns translation with title "foo"

Since write is also defined, you can easily write as well.

Also, locales is simple to define, and reads very natural:

def locales
  map(&:locale)
end

I may be overthinking this a bit, but it feels like this is the right balance. If post.title_backend were renamed to post.title_translations, then this would feel all the more natural.

shioyama added some commits Aug 12, 2017

Fix docs
[ci skip]

@shioyama shioyama merged commit 12e8bcb into master Aug 12, 2017

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@shioyama shioyama deleted the list_locales branch Aug 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment