Make localize_entry localize nested entries and maintain hash base class. #131
Conversation
Entry, if coming from the contentful datastore, should be Thor::CoreExt::HashWithIndifferentAccess which has access via both `entry[:symbol]` and `entry['string']`.
Not sure what nested hashes would be coming from contentful – are there 1:1 relationships?
Not sure what Ruby's typical syntax is for this, didn't fail for me though.
lib/contentful_middleman/helpers.rb
Outdated
| return value.fetch(locale) if value.key? locale | ||
| return value.fetch(fallback_locale) if value.key? fallback_locale | ||
| end | ||
| value = value.fetch(locale) if value.respond_to?(:fetch) && value.respond_to?(:key) && value.key?(locale) |
There was a problem hiding this comment.
key checks are unnecessary and should be key? if they were.
The call to localize_array and localize_entry are totally spot-on. Hadn't tested nested localized items, and it's a great addition.
There was a problem hiding this comment.
I changed it to value.respond_to?(:key?).
The issue I was trying to avoid with this is if value is passed an Array (shouldn't happen aside from user error?) or value becomes an Array via value = value.fetch(locale), Arrays do have fetch on them, but not key?, so it would throw a NoMethodError if it matched on the locale key.
Not sure which syntax is more ruby-like – I'd write this differently in Javascript:
if value.respond_to?(:fetch) && value.respond_to?(:key?)
if value.key?(locale)
value = value.fetch(locale)
elsif value.key?(fallback_locale)
value = value.fetch(fallback_locale)
end
end
or
value = value.fetch(locale) if value.respond_to?(:fetch) && value.respond_to?(:key?) && value.key?(locale)
value = value.fetch(fallback_locale) if value.respond_to?(:fetch) && value.respond_to?(:key?) && value.key?(fallback_locale)
I re-committed the more succinct syntax.
lib/contentful_middleman/helpers.rb
Outdated
| @@ -7,7 +7,8 @@ def contentful_instances | |||
| end | |||
|
|
|||
| def localize_entry(entry, locale, fallback_locale='en-US') | |||
| localized_entry = {} | |||
| localized_entry = entry.class.new | |||
| localized_entry = {} unless localized_entry.is_a? ::Hash | |||
There was a problem hiding this comment.
This can be changed to
localized_entry = ::Thor::CoreExt::HashWithIndifferentAccess.newAs it is a superclass of ::Hash, there should be absolutely no difference with the current implementation.
Just make sure to add the explicit require at the top of the file.
There was a problem hiding this comment.
Great, wasn't sure on that.
Did you want Thor in the gemfile, or happy with it being a very integral part of middleman-core? If Middleman dropped Thor, plenty of this might break anyways..
We're using the `key?`, not the `key` method following this.
…ia symbol. As a result of the changes from adding Thor, we want to make sure symbols still work.
|
Forgot to re-run tests after review changes. Fixed test and added a test to ensure both symbols and strings work as accessors on |
|
Looks good. Merging, thanks for the contribution. |
kylorhall
deleted the
allow-localize_entries-to-localize-nested-entries
branch
…ass. (#131) * Continue to localize further arrays and hashes after localizing the value. * Adds spec for nested_field. * Use the same class as the entry, with a fallback to hash. Entry, if coming from the contentful datastore, should be Thor::CoreExt::HashWithIndifferentAccess which has access via both `entry[:symbol]` and `entry['string']`. * Adds a test for a nested hash as well. Not sure what nested hashes would be coming from contentful – are there 1:1 relationships? * Cleanup trailing commas. Not sure what Ruby's typical syntax is for this, didn't fail for me though. * Change `respond_to?(:key)` to `respond_to?(:key?)` We're using the `key?`, not the `key` method following this. * Use Thor's hash directly here. * Thor seems to have changed this comparison from symbols to strings keys. * Adds tests to validate that the localized_entry is still accessible via symbol. As a result of the changes from adding Thor, we want to make sure symbols still work.
|
This has been released for version Cheers |
Aims to fix my issue #130. Seems to work fine for my use case.
Ruby isn't really my language, so something like
localized_entry = entry.class.new(to create a new instance of the same type of the entry) may be a horrible idea..