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

Merge values for default locale and current locale #58

Merged
merged 3 commits into from
May 11, 2015

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented May 5, 2015

This creates a behaviour more in line with the delivery API. This seems to be more what people expect.

Example of old behaviour:

cat = space.entries.find('nyancat')
puts cat.fields
# {:name=>"Nyan Cat", :likes=>["rainbows", "fish"], :color=>"rainbow", :bestFriend=>{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"happycat"}}, :birthday=>#<DateTime: 2011-04-04T22:00:00+00:00 ((2455656j,79200s,0n),+0s,2299161j)>, :lives=>1337, :image=>{"sys"=>{"type"=>"Link", "linkType"=>"Asset", "id"=>"nyancat"}}}
cat.locale = 'tlh'
puts cat.fields
# {:name=>"Nyan vIghro'", :color=>"yolo"}

Example of new behaviour:

cat = space.entries.find('nyancat')
puts cat.fields
# {:name=>"Nyan Cat", :likes=>["rainbows", "fish"], :color=>"rainbow", :bestFriend=>{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"happycat"}}, :birthday=>#<DateTime: 2011-04-04T22:00:00+00:00 ((2455656j,79200s,0n),+0s,2299161j)>, :lives=>1337, :image=>{"sys"=>{"type"=>"Link", "linkType"=>"Asset", "id"=>"nyancat"}}}
cat.locale = 'tlh'
puts cat.fields
# {:name=>"Nyan vIghro'", :likes=>["rainbows", "fish"], :color=>"yolo", :bestFriend=>{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"happycat"}}, :birthday=>#<DateTime: 2011-04-04T22:00:00+00:00 ((2455656j,79200s,0n),+0s,2299161j)>, :lives=>1337, :image=>{"sys"=>{"type"=>"Link", "linkType"=>"Asset", "id"=>"nyancat"}}}

I'm unsure which one is "better", @pxlpnk @grncdr what do you think?

This creates a behaviour more in line with the delivery API.
@pxlpnk
Copy link
Contributor

pxlpnk commented May 8, 2015

👍

@pxlpnk
Copy link
Contributor

pxlpnk commented May 8, 2015

Can you add the update to the changelog then we are good to merge this.

@grncdr
Copy link
Contributor

grncdr commented May 8, 2015

In the short term I think this change makes sense.

If breaking backwards compatibility is on the table, I'd really prefer that we remove this weird side-effecting locale=(code) method entirely. Instead we should just make locales explicit in the API with an optional parameter to fields, so usage would look like:

cat = space.entries.find('nyancat')
puts cat.fields
# {:name=>"Nyan Cat", :likes=>["rainbows", "fish"], :color=>"rainbow", :bestFriend=>{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"happycat"}}, :birthday=>#<DateTime: 2011-04-04T22:00:00+00:00 ((2455656j,79200s,0n),+0s,2299161j)>, :lives=>1337, :image=>{"sys"=>{"type"=>"Link", "linkType"=>"Asset", "id"=>"nyancat"}}}

puts cat.fields('tlh')
# {:name=>"Nyan vIghro'", :likes=>["rainbows", "fish"], :color=>"yolo", :bestFriend=>{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"happycat"}}, :birthday=>#<DateTime: 2011-04-04T22:00:00+00:00 ((2455656j,79200s,0n),+0s,2299161j)>, :lives=>1337, :image=>{"sys"=>{"type"=>"Link", "linkType"=>"Asset", "id"=>"nyancat"}}}

puts cat.fields('tlh', default_locale: false)
# {:name=>"Nyan vIghro'", :color=>"yolo"}

@neonichu
Copy link
Contributor Author

neonichu commented May 8, 2015

I think that way of using it also works. Personally, I think the locale parameter is good for having a scope in which you deal with just one language.

neonichu added a commit that referenced this pull request May 8, 2015
@pxlpnk
Copy link
Contributor

pxlpnk commented May 8, 2015

The change would just allow the user to access all the fields. Also when they would change the name for example and save it again, the other fields would be removed as they are not included in the request.
So it actually fixes a bug.

@asymmetric
Copy link

@grncdr I'm actually quite happy with the way it works now - keeps the operation of changing locale isolated.

@neonichu neonichu force-pushed the fallback-to-default-locale branch from 76256f3 to bd81af8 Compare May 8, 2015 13:15
@grncdr
Copy link
Contributor

grncdr commented May 8, 2015

Looks good to merge from my side.

@neonichu neonichu force-pushed the fallback-to-default-locale branch from 3250c29 to b5d0fbc Compare May 11, 2015 12:03
pxlpnk added a commit that referenced this pull request May 11, 2015
Merge values for default locale and current locale
@pxlpnk pxlpnk merged commit 5a6be4b into master May 11, 2015
@pxlpnk pxlpnk deleted the fallback-to-default-locale branch May 11, 2015 13:33
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