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

Allow i18n of colander error messages #206

Merged
merged 4 commits into from
Jan 18, 2016

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2013

Fix #200

After investigating the issue further, I realized that not everything can be done by Colander. Colander can only provide translation strings but it cannot perform the translation itself because it doesn't depend on Pyramid and doesn't know about localizers. As suggested earlier, I've proposed a change to Colander so that we can pass a translation callable to the asdict method: Pylons/colander#157

However it's still up to Cornice to provide a callback that is aware of the current locale.
Since Cornice aims at following HTTP conventions, I've added support for the HTTP Accept-Language header. I've also added some configuration code that sets up a localizer depending on configuration variables based on suggestions from the Pyramid documentation:

@@ -32,6 +33,17 @@ def add_apidoc(config, pattern, func, service, **kwargs):
info['func'] = func


def set_localizer_for_languages(event, available_languages,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a docstring here?

@almet
Copy link
Contributor

almet commented Dec 16, 2013

This approach seems reasonable to me.

@almet
Copy link
Contributor

almet commented Dec 16, 2013

Good work :)

@ghost
Copy link
Author

ghost commented Jan 8, 2014

colander:locale/, refers to localization files that ship with Colander. Cornice users don't need to create any particular directory.

Regarding lazily setting up localization, do you say that for performance reasons? I suppose that instead of setting a localizer for every request, we could set it up only in the case where there are validation errors. Would that be better?

Couple of changes I just did:

  • now catch the import error that may be raised when trying to add Colander translation directory when Colander is not installed.
  • added docstrings and documentation

FYI, a colander maintainer seems favorable to my pull request Pylons/colander#157 Hopefully it will get merged soon, but you probably cannot merge the current pull request until a new version of Colander including that change is released on PyPI.

@ghost
Copy link
Author

ghost commented Mar 12, 2014

The required Colander change has been merged to master. Now waiting for the next Colander release before we can consider merging this pull request.

@leplatrem
Copy link
Contributor

I hadn't seen this PR, and had some of this stuff re-implemented into Daybed : spiral-project/daybed#258

Alex's code is a lot better :) and it makes of course more sense to assemble all these in Cornice instead of Daybed :)

@ghost
Copy link
Author

ghost commented Dec 27, 2014

There's been a new colander release about a month ago, but my change hasn't been included in it. It's still in the next release feature list.

@Natim
Copy link
Contributor

Natim commented Feb 24, 2015

It look like it has been merged. Can we rebase that code and get it merged?

@ghost
Copy link
Author

ghost commented Jul 2, 2015

@Natim Colander changes have been merged but not released yet. Scheduled for next release according to https://github.com/Pylons/colander/blob/master/CHANGES.rst

I just rebased this PR to keep it in sync with master (had a couple of minor conflicts) but it still cannot be merged until required Colander changes get released.

@lmctv
Copy link

lmctv commented Jan 16, 2016

Colander 1.1 has been released on PyPI, with Pylons/colander#157 merged.

@Natim
Copy link
Contributor

Natim commented Jan 16, 2016

Thank you guys :)

@ghost ghost changed the title [Do not merge] Allow i18n of colander error messages Allow i18n of colander error messages Jan 16, 2016
@ghost
Copy link
Author

ghost commented Jan 16, 2016

I just rebased on master, resolved conflicts and pushed. I'm not sure why GitHub says it has conflicts. I just tried manually merging this branch into master and it had no conflicts.

@Natim How does it look on your end?

@lmctv
Copy link

lmctv commented Jan 16, 2016

From github's Pull Request page, it looks like you merged master instead of rebasing the originale commits:

screenshot_2016-01-16-17-54-04 2

@lmctv
Copy link

lmctv commented Jan 16, 2016

Maybe you forgot to update your local master branch before rebasing your pull request onto master?

makina_cornice

Furthermore, I think you should drop the "Update documentation version number" makinacorpus/cornice@186fd8b and leave alone docs/source/conf.py .

@ghost ghost force-pushed the colander-i18n branch 2 times, most recently from b6a2a40 to a5f3234 Compare January 16, 2016 20:54
@ghost
Copy link
Author

ghost commented Jan 16, 2016

@lmctv Thanks a lot, I totally forgot to update my local master before rebasing. Should be fine to merge now.

To enable localization of Colander error messages, you must set
`available_languages <http://docs.pylonsproject.org/projects/pyramid/en/1.3-branch/narr/i18n.html#detecting-available-languages>`_ in your settings.
You may also set `pyramid.default_locale_name <http://docs.pylonsproject.org/projects/pyramid/en/1.3-branch/narr/environment.html#default-locale-name-setting>`_.

Copy link

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@lmctv good catch, done

Natim added a commit that referenced this pull request Jan 18, 2016
Allow i18n of colander error messages
@Natim Natim merged commit 81b73c2 into Cornices:master Jan 18, 2016
@Natim
Copy link
Contributor

Natim commented Jan 18, 2016

Thansk @amarandon

@almet
Copy link
Contributor

almet commented Jan 18, 2016

This was a loooong wait. Thanks again.

Natim added a commit that referenced this pull request Jan 18, 2016
Remove superfluous call to get_localizer (ref #206)
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.

Allow to translate errorr messages originating from colander schemas
5 participants