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

Added i18w support + danish translation. #27

Closed
wants to merge 6 commits into from

Conversation

LeonhardPrintz
Copy link

I'm resubmitting this after squashing some fixup commits.


#: ../flask_security/templates/security/email/change_notice.html:4
msgid "click here to reset it"
msgstr "click her for at ændre den"
Copy link
Member

Choose a reason for hiding this comment

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

click -> Klik


#: ../flask_security/templates/security/send_confirmation.html:3
msgid "Resend confirmation instructions"
msgstr "Gensend bekræftigelses instruktioner"
Copy link
Member

Choose a reason for hiding this comment

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

Gensend bekræftelses instruktioner
?


#: ../flask_security/templates/security/_menu.html:9
msgid "Forgot password"
msgstr "Glemt Adgangskode"
Copy link
Member

Choose a reason for hiding this comment

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

Glemt din adgangskode

#!/bin/sh
pybabel extract -F babel.ini -k _gettext -k _ngettext -k lazy_gettext -o security.pot --project Flask-Security ../flask_security
pybabel compile -f -D security -d ../flask_security/translations/

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the extra empty line?

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

'EMAIL_SUBJECT_PASSWORD_NOTICE': 'Your password has been reset',
'EMAIL_SUBJECT_PASSWORD_CHANGE_NOTICE': 'Your password has been changed',
'EMAIL_SUBJECT_PASSWORD_RESET': 'Password reset instructions',
'DEFAULT_HTTP_AUTH_REALM': _('Login Required'),
Copy link
Member

Choose a reason for hiding this comment

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

IMHO in this file all strings should be only marked for translation and the gettext should be call in place where these strings are used so developers can provide text only configuration for their application.

Copy link
Author

Choose a reason for hiding this comment

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

You mean if one specified feks EMAIL_SUBJECT_PASSWORD_NOTICE, as a normal text string in a config file, it should still be translated by gettext?

Or do you mean that using a normal string for EMAIL_SUBJECT_PASSWORD_NOTICE wouldn't result in errors? It shouldn't as far as I know as lazy_gettexts are only evaluated when used, so they behave as if they were ordinary strings.

Copy link
Member

Choose a reason for hiding this comment

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

lazy_gettexts are only evaluated when used ...

This is a problem for configuration strings provided manually as str. They would not be translated when used.

Copy link
Author

Choose a reason for hiding this comment

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

That is true. I guess this is ultimately a use-case philosophical discussion. People following the Flask Mega-Tutorial (myself included) wrap all configuration strings to be translated in lazy_gettext. This would work as well, and the translated files would end up in that person .mo files.

I can spend some time on an alternative solution if you're sure. Perhaps adding a callback somewhere. But I think its more complexity for no clear benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, I should have elaborate a bit more at the beginning. There are two steps for making the extension configurable and translation friendly.

  1. Make sure that all strings are properly extracted by Babel (usually done by _ or *gettext function).
  2. Render translated string in UI.

For the first step we need to make sure that default configuration options are marked. This can be simply done by an identity function (def _(x): return x). In order to make second step correctly we need to make sure that the gettext (or it's lazy equivalent) function is call on each string that a user can see.

What if developer provides different configuration for Flask-Security messages?
First, (s)he needs to make sure that these strings are extracted from the sources and included in flask_security.mo that it's loaded by Flask-BabelEx.

I hope I have explained the problems well.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks it does, the login_manager from Flask-Login has a gettext callback that can be used, and I just have to find something similar and appropriate for the forms, and make sure its all done properly everywhere.

My test bed is up and running, so I can figure things out. A bit busy this week, but I'll have something by the weekend.

Thanks for helping out a guy with his first open source pull request. Kinda exciting. :)

Copy link
Member

Choose a reason for hiding this comment

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

@LeonhardPrintz you are welcome 👍 I hope we can find an elegant solution. If you have any questions you can ping me here or on Gitter.

class CustomDomain(Domain):
def __init__(self):
super(CustomDomain, self).__init__(
translations.__path__[0], domain='security')
Copy link
Member

Choose a reason for hiding this comment

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

Why do we choose domain name 'security' instead of 'flask_security'?

Copy link
Author

Choose a reason for hiding this comment

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

This might be a bug, I'll make a small demo and see if anything is broken.

Copy link
Author

Choose a reason for hiding this comment

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

It was, fixed in next commit.

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

3 participants