Skip to content

Allow users to programmatically override default i18n strings #91

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

Merged
merged 3 commits into from
Oct 9, 2017
Merged

Allow users to programmatically override default i18n strings #91

merged 3 commits into from
Oct 9, 2017

Conversation

ALCC01
Copy link
Contributor

@ALCC01 ALCC01 commented Oct 6, 2017

Implements the API set out in #47 to allow botogram users to override i18n messages without the need to recompile the .po files. This change may conflict with #85, but the PR looks pretty dead. This also contributes towards the completion of the #46 roadmap.

When the FrozenBot._ method is called it will first look up the msgid in the i18n_override map, before falling back to _lang_inst.gettext. This actually allows users to define custom messages stored in the bot instance and is independent from the language the bot is using.

While not strictly related to #47, .po files have been rebuilt from sources as they were missing some new messages and used outdated references.

Copy link
Contributor

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, and welcome!

There are a few nitpicks in the code you sent, but more importantly the new feature is missing documentation and a changelog line. You can either just add the API docs for the attribute, or also write a narrative chapter about i18n.

Looking forward to merge this!

@@ -82,7 +83,7 @@ def __reduce__(self):
self.validate_callback_signatures, self.process_backlog, self.lang,
self.itself, self._commands_re, self._commands, self._chains,
self._scheduler, self._main_component_id, self._bot_id,
self._shared_memory, self._update_processors,
self._shared_memory, self._update_processors, self.override_i18n
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a comma after the last element (so it's easier to add new arguments here)

@@ -76,3 +77,17 @@ def test_bot_freeze(bot):
frozen = bot.freeze()

assert bot == frozen

def test_i18n_override(bot):
default_message = botogram.utils.get_language("en").gettext("Use /help to get a list of all the commands.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The line is too long, can you make it fit into 80 chars (maybe splitting it)?

default_message: override_message
}

assert bot._("Use /help to get a list of all the commands.") == "git gud"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you should use the variable you defined earlier.


bot.override_i18n = {}

assert bot._("Use /help to get a list of all the commands.") == default_message
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is also too long.

@ALCC01
Copy link
Contributor Author

ALCC01 commented Oct 9, 2017

6bbc17e should address the changes you requested. I was thinking of opening another PR to work on the narrative documentation since it fits with other changes proposed in #33. I was also hesitant about writing anything at all about i18n since #85 is still open.

A dictionary that allows to override default i18n messages by associating
the default ``msgid`` string of a message with its alternative.

.. versionadded:: 0.4.1
Copy link
Member

@paolobarbolini paolobarbolini Oct 9, 2017

Choose a reason for hiding this comment

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

Since it's a new feature it probably will be added in 0.5

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I'll tweak the changelog myself after merging, don't worry.

Copy link
Contributor

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

LGTM

@pietroalbini pietroalbini merged commit 6bbc17e into python-botogram:master Oct 9, 2017
@pietroalbini
Copy link
Contributor

Thank you for this pull request, I merged it!

About the narrative documentation, #85 isn't a blocker for that: the language_code thing can be added later when it's implemented. And thanks for considering writing it, more documentation is badly needed.

@ALCC01 ALCC01 deleted the i18n_override branch October 10, 2017 14:33
@ALCC01
Copy link
Contributor Author

ALCC01 commented Oct 11, 2017

I think #47 should be closed and #46 updated to reflect the changes that have been merged.

@pietroalbini
Copy link
Contributor

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants