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

Unify Locale Handling #78

Merged
merged 1 commit into from
Dec 3, 2015
Merged

Unify Locale Handling #78

merged 1 commit into from
Dec 3, 2015

Conversation

dlitvakb
Copy link
Contributor

@dlitvakb dlitvakb commented Dec 2, 2015

Fixes #73

expect(cat.fields[:name][:'en-US']).to eq "Nyan Cat"
expect(cat.fields[:name][:'es']).to eq "Gato Nyan"
expect(cat.fields('en-US')[:name]).to eq "Nyan Cat"
expect(cat.fields('es')[:name]).to eq "Gato Nyan"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure why we are reversing the order of parameters here? It'll be a breaking change.

In any case, the locale parameter should be a symbol, otherwise it'll be inconsistent with field_with_locales.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually the expected behavior of fields, I was omitting that behavior in order to be able to serialize correctly in contentful_middleman.

This, though breaking is undocumented anywhere else than this test, and is fixing the dispaired behavior.

fields_with_locales is the in place replacement for this functionality, and it is documented properly

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, this is actually the documented way, at least for sync: https://github.com/contentful/contentful.rb/blame/master/README.md#L327

I still think we should make both methods consistent in their use of symbols vs. strings for the locales, otherwise it'll be confusing.

@neonichu
Copy link
Contributor

neonichu commented Dec 3, 2015

neonichu added a commit that referenced this pull request Dec 3, 2015
@neonichu neonichu merged commit 6d910c2 into master Dec 3, 2015
@neonichu neonichu deleted the dl/unify-locale-handling branch December 3, 2015 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants