Skip to content

Added **kwargs support for menu translations #43

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

Closed
wants to merge 3 commits into from
Closed

Added **kwargs support for menu translations #43

wants to merge 3 commits into from

Conversation

Mahi
Copy link
Contributor

@Mahi Mahi commented Mar 4, 2015

All menus now support keyword arguments when translating them. This
includes _MenuData classes (texts and options), descriptions, and
titles.

Mahi added 3 commits March 4, 2015 02:34
All menus now support keyword arguments when translating them. This
includes _MenuData classes (texts and options), descriptions, and
titles.
@Ayuto
Copy link
Member

Ayuto commented Mar 4, 2015

There are a couple of issues with this implementation. At first the name "kwargs" is very ambiguous. A better name would be "tokens". The description would also need to be changed. However, this isn't a real problem, but one problem I see here is that you can't use "**kwargs", because every method has keywords. That would result in unusable token names like "value", "selectable", etc. Of course, this could be fixed by just using a new keyword "tokens=None".

However, I don't think this is the right approach, because these tokens are just statically passed to the translator function. If you want to update the tokens you would need to do something like this:

menu.description_tokens.update(...)

But that's just unecessary, because you can just do this:

menu.description.tokens.update(...)

So, the real problem we have here, is that you can't get a TranslationStrings object with an initialized tokens dictionary. We could simply work around that by adding this method to the LangStrings class:

    def get_translation_strings(self, key, **tokens):
        strings = self[key]
        strings.update(tokens)
        return strings

To show both methods in contrast:

# Your method
PagedMenu(description=my_strings['test'], description_tokens=dict(token1='abc', token2='bla'))

# Better, imo
PagedMenu(description=my_strings.get_translation_strings('test', token1='abc', token2='bla'))

@Mahi
Copy link
Contributor Author

Mahi commented Mar 4, 2015

Oh, right :) Well I wasn't actually expecting this exact code to get accepted, I just gave my shot and was hoping people would come up with something. Apparently your solution was nothing similar, but much better.

However, it would be cool to get some sort of translation keywords for menus. I currently use the kwargs=None and self.kwargs = kwargs or {} solution, waiting for something better to show up.

@Ayuto
Copy link
Member

Ayuto commented Mar 4, 2015

For now you can just move this method somewhere in your code. You can then do something like this:

def get_translation_strings(self, key, **tokens):
    strings = self[key]
    strings.update(tokens)
    return strings

menu = PagedMenu(description=get_translation_strings(my_strings, 'test', token1='abc'))

But I would still like to hear a third or fourth opinion on this.

@Mahi
Copy link
Contributor Author

Mahi commented Mar 5, 2015

Yeah I figured, I already did that :) After re-reading your first comment a few times, that is.

@Ayuto
Copy link
Member

Ayuto commented Mar 14, 2015

Since there was no more feedback, I added the get_strings() method. 9507fd6

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.

2 participants