[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation
Review Request #11880 — Created Nov. 21, 2021 and updated
[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation
=> Local testing
=> Unit tests added for emoji registry methods
Description | From | Last Updated |
---|---|---|
E501 line too long (86 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E261 at least two spaces before inline comment |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W291 trailing whitespace |
![]() |
|
E117 over-indented |
![]() |
|
E501 line too long (102 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E117 over-indented |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E201 whitespace after '{' |
![]() |
|
E501 line too long (97 > 79 characters) |
![]() |
|
E202 whitespace before '}' |
![]() |
|
W292 no newline at end of file |
![]() |
|
E501 line too long (32480 > 79 characters) |
![]() |
|
W292 no newline at end of file |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (107 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
This isn't something we need to worry about. Djblets 2.x+ is Django 1.11+ only. |
|
|
A few things here with imports: The first import should always be unicode_literals as well as any other __future__ things … |
|
|
Parens aren't necessary here |
|
|
No need to define the local variable here. Just assign directly into the dict key. |
|
|
This isn't a helpful thing to have here--it's kind of obvious what these are. |
|
|
Combine these lines ("""Add the src attributes...) |
|
|
For docstrings, the first line should be a summary, and additional info should go in other paragraphs. At the very … |
|
|
All identifiers in python (except for class names) should use underscores rather than camelCase. |
|
|
This isn't particularly efficient (.index() has to iterate the list every time it's called). When we're building the sort order, … |
|
|
There's a python syntax trick that lets us avoid defining a method for this: combined_indices = {**i1['emoji'], **i2['emoji']} combined_aliases = … |
|
|
Python style says that this should be named get_unicode_sort_order |
|
|
We really should make this process the .txt file directly from the unicode consortium rather than list it out ourselves. … |
|
|
E501 line too long (32480 > 79 characters) |
![]() |
|
Same comments about import orders. |
|
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
All string literals should use single quotes unless there's a good reason not to (for example, a single quote within … |
|
|
Comments should use #, not triple-quoted strings. |
|
|
E501 line too long (107 > 79 characters) |
![]() |
|
Based on my reading of the code, I think this can just be: index = emoji_registry.get_emoji_index(for_extension=True) new_kwargs['emoji_index'] = lambda: index |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
Make sure to add info. |
|
|
This needs a "Returns" section. |
|
|
This needs a "Returns" section. |
|
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W292 no newline at end of file |
![]() |
|
F401 'kgb.SpyAgency' imported but unused |
![]() |
|
F401 'djblets.siteconfig.models.SiteConfiguration' imported but unused |
![]() |
|
F401 'djblets.testing.testcases.TestCase' imported but unused |
![]() |
|
E303 too many blank lines (3) |
![]() |
|
W391 blank line at end of file |
![]() |
|
F401 'collections.OrderedDict' imported but unused |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W292 no newline at end of file |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E303 too many blank lines (3) |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E501 line too long (97 > 79 characters) |
![]() |
|
E303 too many blank lines (3) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
F401 'pytz' imported but unused |
![]() |
|
E501 line too long (97 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E501 line too long (101 > 79 characters) |
![]() |
|
E501 line too long (101 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (95 > 79 characters) |
![]() |
|
E501 line too long (98 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+446) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
-
-
-
-
-
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 2) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 2) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 2) E501 line too long (107 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 2) E501 line too long (80 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 2) E303 too many blank lines (2)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+449) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) E501 line too long (107 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) E501 line too long (80 > 79 characters)
-
OK, I added a handful of comments (some of these are style things that apply throughout the change and I just marked one instance).
The biggest thing is how we're dealing with the sort order. We really should just take the .txt file from unicode.org and bundle it, and then have our code process that file to build the sort order. That way, when future versions of Unicode come out, we can just drop in a new file.
-
djblets/emojis/apps.py (Diff revision 3) This isn't something we need to worry about. Djblets 2.x+ is Django 1.11+ only.
-
djblets/emojis/registry.py (Diff revision 3) A few things here with imports:
The first import should always be
unicode_literals
as well as any other__future__
things that are necessary. After that there are three groups: standard library, third-party, and the same module. Within each group we alphabetize. We also don't use the relative import syntax (yet).from __future__ import unicode_literals import datetime import json from collections import OrderedDict import pytz from djblets.emojis.unicode_sort_order import get_unicode_sort_order from djblets.siteconfig.models import SiteConfiguration
-
-
djblets/emojis/registry.py (Diff revision 3) No need to define the local variable here. Just assign directly into the dict key.
-
djblets/emojis/registry.py (Diff revision 3) This isn't a helpful thing to have here--it's kind of obvious what these are.
-
-
djblets/emojis/registry.py (Diff revision 3) For docstrings, the first line should be a summary, and additional info should go in other paragraphs. At the very least put a blank line between these two and combine the """ with the summary line.
-
djblets/emojis/registry.py (Diff revision 3) All identifiers in python (except for class names) should use underscores rather than camelCase.
-
djblets/emojis/registry.py (Diff revision 3) This isn't particularly efficient (
.index()
has to iterate the list every time it's called). When we're building the sort order, let's create a dict mapping the codepoint to its index value. -
djblets/emojis/registry.py (Diff revision 3) There's a python syntax trick that lets us avoid defining a method for this:
combined_indices = {**i1['emoji'], **i2['emoji']} combined_aliases = {**i1['aliases'], **i2['aliases']}
-
djblets/emojis/unicode_sort_order.py (Diff revision 3) Python style says that this should be named
get_unicode_sort_order
-
djblets/emojis/unicode_sort_order.py (Diff revision 3) We really should make this process the .txt file directly from the unicode consortium rather than list it out ourselves. That way it's much easier to update.
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) Same comments about import orders.
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) All string literals should use single quotes unless there's a good reason not to (for example, a single quote within the string content itself).
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) Comments should use #, not triple-quoted strings.
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) Based on my reading of the code, I think this can just be:
index = emoji_registry.get_emoji_index(for_extension=True) new_kwargs['emoji_index'] = lambda: index
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) This needs a "Returns" section.
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3) This needs a "Returns" section.

Change Summary:
David's comments + changes to how emoji index is retrieved for optimization
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+4217) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
-
-
-
-
-
-
djblets/emojis/tests.py (Diff revision 4) F401 'djblets.siteconfig.models.SiteConfiguration' imported but unused
-
djblets/emojis/tests.py (Diff revision 4) F401 'djblets.testing.testcases.TestCase' imported but unused
-
-
-
djblets/emojis/unicode_sort_order.py (Diff revision 4) F401 'collections.OrderedDict' imported but unused
-
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E302 expected 2 blank lines, found 1
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 4) E501 line too long (80 > 79 characters)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+4218) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E302 expected 2 blank lines, found 1
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 5) E501 line too long (80 > 79 characters)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+4218) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 6) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 6) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 6) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 6) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 6) E501 line too long (80 > 79 characters)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+4295) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 7) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 7) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 7) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 7) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 7) E501 line too long (80 > 79 characters)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+4389) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
-
-
-
-
-
-
-
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 8) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 8) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 8) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 8) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 8) E501 line too long (80 > 79 characters)

Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 9 (+4386) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 9) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 9) E501 line too long (81 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 9) E501 line too long (95 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 9) E501 line too long (98 > 79 characters)
-
djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 9) E501 line too long (80 > 79 characters)