Migrate emoji storage to group aliases together for each canonical name. #1084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zee-bit
added
further discussion required
neiljp
reviewed
Jul 16, 2021
zee-bit
force-pushed
the
update-emoji-storage-by-name
branch
from
2fdf218
to
46a7cf3
Compare
4 tasks
preetmishra
reviewed
Jul 18, 2021
There was a problem hiding this comment.
@zee-bit Thanks for working on this. This looks good overall. 👍 Though, as there has been a discussion on CZO, generating the new file midway or separately in a commit would bring discrepancies in model.py.
neiljp
added
PR awaiting update
zee-bit
force-pushed
the
update-emoji-storage-by-name
branch
2 times, most recently
from
d7339ae
to
1a32875
Compare
zee-bit
added
PR needs review
zee-bit
force-pushed
the
update-emoji-storage-by-name
branch
2 times, most recently
from
94c5569
to
23a95d5
Compare
neiljp
reviewed
Jul 22, 2021
There was a problem hiding this comment.
@zee-bit You've got the correct principle here. To ease the migration and avoid the mid-commit regression (which should be breaking tests?), a possible refactoring would be to first adjust the emoji data generation function to return an additional set of data ie. the pre-prepared emoji names, adapting the autocomplete to use it. You can then regenerate the emoji file and the emoji manipulation internal to the model at the same time in the next commit.
zulipterminal/model.py
Outdated
Comment on lines
180
to
183
| self.active_emoji_data = self.generate_all_emoji_data( | ||
| self.initial_data["realm_emoji"] | ||
| ) | ||
| self.all_emoji_names = self.generate_all_emoji_names() |
There was a problem hiding this comment.
We update emoji when we get a realm emoji change event, so we at least want to call both in each case, or have one method that returns a tuple and updates both at the same time.
neiljp
added
PR awaiting update
This commit adds a new instance variable in model that stores the list of all emoji names generated during model initialization by the generate_emoji_data function. The generate_emoji_data function now return a tuple of all active emoji data and all emoji names sorted. This also migrates the autocomplete emoji function to use the list of all_emoji_names returned from this, instead of generating it multiple times during runtime whenever autocomplete emoji is called. Tests amended.
This commit updates the convert-unicode-emoji-data script to store all the unicode emoji codepoints in their extended format as received from the server. This script will map each emoji_name (aka canonical_name) to its data containing the emoji_code and aliases that share the same emoji_code. We sort this dictionary in ascending order of key (emoji_name) and store it as an OrderedDict to maintain the order of sorting. We also turn off black formatting by wrapping the file with fmt: off/on to disable black from formatting the dictionaries afterwards - since that is a generated file, it should not be modified.
E501 ignores max-line-length property - this is already defined by black. We add this error code in flake8's ignore list in setup.cfg file.
This commit generates the EMOJI_DATA from tools/convert-unicode-emoji-data in the new format that maps each emoji_name to its corresponding emoji's code, and aliases that share the same emoji_code, stored in an OrderedDict. The EMOJI_DATA is sorted in ascending order of emoji_name and this ordering is maintained via OrderedDict. The generated EMOJI_DATA is stored in unicode_emojis.py file. This also updates the helper function in model that generates all emoji data to follow and adapt to this new format of storing emoji's and updates the EmojiData type to indicate each emoji now includes an aliases field. This also migrates the emoji fixtures in conftest i.e. realm_emoji_data, unicode_emojis and zulip_emoji to the new format of storing emoji_data. Tests amended.
zee-bit
force-pushed
the
update-emoji-storage-by-name
branch
from
23a95d5
to
0aecbeb
Compare
zee-bit
added
PR needs review
|
@zee-bit This flows much cleaner with no mid-PR regressions - great! Merging this now 🎉 |
neiljp
added
area: refactoring
and removed
further discussion required
|
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Migrates our emoji storage structure in
unicode_emojis.pyto a new structure wherefor each
canonical_namewe keep its aliases grouped together, instead of consideringthem as separate emojis.
Tested?
Notes & Questions
I need to test the reactions PR on top of this to determine the authenticity of this migration.
Interactions