Skip to content

Get the msgids directly from the MessageIdStore #5606

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

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

We don't need to create message definitions to get their msgids.

The level of indirection may suggest we need to refactor a
little here. Maybe we actually need message definition only
when we want to display the messages.

Follow-up to #5605

We don't need to create message definitions to get their msgids.

The level of indirection may suggest we need to refactor a
little here. Maybe we actually need message definition only
when we want to display the messages.
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component performance labels Dec 28, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 28, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1631086682

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0004%) to 93.717%

Totals Coverage Status
Change from base Build 1630989169: -0.0004%
Covered Lines: 14334
Relevant Lines: 15295

πŸ’› - Coveralls

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Is there a difference between the message definitions and the "active" message definitions?

@Pierre-Sassoulas
Copy link
Member Author

I'll let you judge, here's the implementation of get_message_definitions πŸ˜„

    def get_message_definitions(self, msgid_or_symbol: str) -> List[MessageDefinition]:
        """Returns the Message definition for either a numeric or symbolic id."""
        return [
            self._messages_definitions[m]
            for m in self.message_id_store.get_active_msgids(msgid_or_symbol)
        ]

I think we discussed typing msgid at some point as msgid are string with a letter and 4 digits and decided against it. This might be a big refactor but using msgid everywhere instead of message definition might be a good idea. We could even transform symbol to msgid early on when it's user input.

@DanielNoord
Copy link
Collaborator

πŸ˜„

The refactor should probably be left for a later point, but this is indeed a good fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants