Skip to content

Support for other TranslationStrings instances in tokens #164

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.

Sign up for GitHub

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
KirillMysnik wants to merge 3 commits into from
Closed

Support for other TranslationStrings instances in tokens #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58eee7a
TranslationStrings now support other TranslationStrings instances in …
Nov 11, 2016
540abcc
Removed duplicate language checks
Nov 11, 2016
8aebe31
Fixed #189
Apr 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion addons/source-python/packages/source-python/auth/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,11 @@ def is_player_authorized(self, index, permission):

:rtype: bool
"""
return permission in self.get_player_permissions(index)
permissions = self.get_player_permissions(index)
if permissions is None:
return False

return permission in permissions

def get_parent_permissions(self, parent_name):
"""Return the parent permissions.
Expand Down
45 changes: 36 additions & 9 deletions addons/source-python/packages/source-python/translations/strings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,6 @@ def _replace_escaped_sequences(given_string):
# Return the replaced string
return given_string

def get_strings(self, key, **tokens):
"""Return a TranslationStrings object with updated tokens."""
strings = self[key]
strings.tokens.update(tokens)
return strings


Copy link
Member

@Ayuto Ayuto Nov 14, 2016

Choose a reason for hiding this comment

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

This method was added because of PR #43.

Copy link
Member Author

@KirillMysnik KirillMysnik Nov 16, 2016

Choose a reason for hiding this comment

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

Okay, now I see where it comes from. But in my opinion, we shouldn't tokenize the original TranslationStrings instances that are stored in LangStrings. How about this:

def get_strings(self, key, **tokens):
    """Return a new TranslationStrings object with updated tokens."""
    strings = TranslationStrings()
    strings.update(self[key])
    strings.tokens.update(tokens)
    return strings

Copy link
Member

@Ayuto Ayuto Nov 16, 2016

Choose a reason for hiding this comment

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

Well, if you want to prevent in-place updates, you would change the __getitem__ method. But actually, that doesn't really matter, because you always pass all tokens, right?

Copy link
Member Author

@KirillMysnik KirillMysnik Nov 17, 2016

Choose a reason for hiding this comment

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

Yes, everyone should probably pass the complete dict of tokens, but with current implementation of get_strings those tokens will get stuck in LangStrings for absolutely no reason.
Regarding to __getitem__, if somebody gets the empty TranslationStrings instance that is stored in LangStrings and does something filthy with its tokens, it's their problem. Creating a new TranslationStrings instance in __getitem__ and then another one while tokenizing it? That's too much of instantiating, I say. Would be nice to hear opinions from other contributors, too. @Mahi, I believe this method originates from your PR?

I also don't see any reason for keeping LangStrings.get_strings now when we have TranslationStrings.tokenized method.

class TranslationStrings(dict):
"""Stores and get language strings for a particular string."""
Expand Down Expand Up @@ -250,11 +244,29 @@ def get_string(self, language=None, **tokens):
# Possibly raise an error silently here
return ''

# Update the stored tokens with the given ones
self.tokens.update(tokens)
# Expose all TranslationStrings instances in self.tokens
exposed_tokens = {}
for token_name, token in self.tokens.items():
if isinstance(token, TranslationStrings):

# Pass additional tokens - these will be used to format
# the string
token = token.get_string(language, **tokens)

exposed_tokens[token_name] = token

# Expose all TranslationsStrings instances in **tokens
Copy link
Member

@Ayuto Ayuto Nov 14, 2016

Choose a reason for hiding this comment

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

I think we should create a helper method for this duplicate. Maybe something like this:

        # Code...

        # Expose all TranslationStrings instances in self.tokens
        exposed_tokens = {}

        self._update_exposed_tokens(exposed_tokens, language, self.tokens, **tokens)
        self._update_exposed_tokens(exposed_tokens, language, tokens)

        return self[language].format(**exposed_tokens)

    @staticmethod
    def _update_exposed_tokens(exposed_tokens, language, tokens, **kwargs):
        for token_name, token in tokens.items():
            if isinstance(token, TranslationStrings):
                # Pass additional tokens - these will be used to format
                # the string
                token = token.get_string(language, **kwargs)

            exposed_tokens[token_name] = token

Copy link
Member Author

@KirillMysnik KirillMysnik Nov 16, 2016

Choose a reason for hiding this comment

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

I'm not sure about this one... Is there a real need to do so? It's impossible to put everything this method does into its name, so anybody reading the code will need to look into _update_exposed_tokens anyways.

Copy link
Member

@Ayuto Ayuto Nov 16, 2016

Choose a reason for hiding this comment

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

There is no need to do that, but it removes duplicated code. And in my opinion it's easier to read the code, because it's easier to see the difference between both calls.

for token_name, token in tokens.items():
if isinstance(token, TranslationStrings):

# Don't pass any additional tokens, the token should either
# be trivial or rely on itself (self.tokens)
token = token.get_string(language)

exposed_tokens[token_name] = token

# Return the formatted message
return self[language].format(**self.tokens)
return self[language].format(**exposed_tokens)

def get_language(self, language):
"""Return the language to be used."""
Expand Down Expand Up @@ -291,5 +303,20 @@ def get_language(self, language):
# Return None as the language, as no language has been found
return None

def tokenized(self, **tokens):
"""Create a new TranslationStrings instance and store tokens in it.

:param dict tokens: Tokens to store in the instance.
:return: New TranslationStrings instance with tokens stored in it.
:rtype: TranslationStrings
"""
result = TranslationStrings()
result.tokens.update(tokens)

Copy link
Member

@Ayuto Ayuto Nov 14, 2016

Choose a reason for hiding this comment

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

This could just be result.update(self). But we might even want to create a deep copy.

Copy link
Member Author

@KirillMysnik KirillMysnik Nov 16, 2016

Choose a reason for hiding this comment

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

Yes, you're right. This must be a leftover from something bigger, but now it can indeed be written as result.update(self).

for key, value in self.items():
result[key] = value

return result

# Get the translations language strings
_translation_strings = LangStrings('_core/translations_strings')