[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation

Review Request #11880 — Created Nov. 21, 2021 and updated

Information

Djblets
release-2.x
a864bc5...

Reviewers

[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation

See Final Report

Image

=> Local testing
=> Unit tests added for emoji registry methods

Description From Last Updated

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E261 at least two spaces before inline comment

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E117 over-indented

reviewbotreviewbot

E501 line too long (102 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E117 over-indented

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E201 whitespace after '{'

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E202 whitespace before '}'

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E501 line too long (32480 > 79 characters)

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (107 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

This isn't something we need to worry about. Djblets 2.x+ is Django 1.11+ only.

daviddavid

A few things here with imports: The first import should always be unicode_literals as well as any other __future__ things …

daviddavid

Parens aren't necessary here

daviddavid

No need to define the local variable here. Just assign directly into the dict key.

daviddavid

This isn't a helpful thing to have here--it's kind of obvious what these are.

daviddavid

Combine these lines ("""Add the src attributes...)

daviddavid

For docstrings, the first line should be a summary, and additional info should go in other paragraphs. At the very …

daviddavid

All identifiers in python (except for class names) should use underscores rather than camelCase.

daviddavid

This isn't particularly efficient (.index() has to iterate the list every time it's called). When we're building the sort order, …

daviddavid

There's a python syntax trick that lets us avoid defining a method for this: combined_indices = {**i1['emoji'], **i2['emoji']} combined_aliases = …

daviddavid

Python style says that this should be named get_unicode_sort_order

daviddavid

We really should make this process the .txt file directly from the unicode consortium rather than list it out ourselves. …

daviddavid

E501 line too long (32480 > 79 characters)

reviewbotreviewbot

Same comments about import orders.

daviddavid

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

All string literals should use single quotes unless there's a good reason not to (for example, a single quote within …

daviddavid

Comments should use #, not triple-quoted strings.

daviddavid

E501 line too long (107 > 79 characters)

reviewbotreviewbot

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

daviddavid

E501 line too long (80 > 79 characters)

reviewbotreviewbot

Make sure to add info.

daviddavid

This needs a "Returns" section.

daviddavid

This needs a "Returns" section.

daviddavid

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

F401 'kgb.SpyAgency' imported but unused

reviewbotreviewbot

F401 'djblets.siteconfig.models.SiteConfiguration' imported but unused

reviewbotreviewbot

F401 'djblets.testing.testcases.TestCase' imported but unused

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'collections.OrderedDict' imported but unused

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

F401 'pytz' imported but unused

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (101 > 79 characters)

reviewbotreviewbot

E501 line too long (101 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (98 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
david
  1. 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.

  2. djblets/emojis/apps.py (Diff revision 3)
     
     
     
     
     
     

    This isn't something we need to worry about. Djblets 2.x+ is Django 1.11+ only.

  3. 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
    
  4. djblets/emojis/registry.py (Diff revision 3)
     
     

    Parens aren't necessary here

  5. djblets/emojis/registry.py (Diff revision 3)
     
     
     
     

    No need to define the local variable here. Just assign directly into the dict key.

  6. djblets/emojis/registry.py (Diff revision 3)
     
     
     
     

    This isn't a helpful thing to have here--it's kind of obvious what these are.

  7. djblets/emojis/registry.py (Diff revision 3)
     
     
     

    Combine these lines ("""Add the src attributes...)

  8. 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.

  9. djblets/emojis/registry.py (Diff revision 3)
     
     

    All identifiers in python (except for class names) should use underscores rather than camelCase.

  10. 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.

  11. 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']}
    
  12. djblets/emojis/unicode_sort_order.py (Diff revision 3)
     
     

    Python style says that this should be named get_unicode_sort_order

  13. 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.

  14. djblets/markdown/extensions/emoji_extension_wrapper.py (Diff revision 3)
     
     
     
     
     
     
     

    Same comments about import orders.

  15. 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).

  16. Comments should use #, not triple-quoted strings.

  17. 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
    
  18. Make sure to add info.

  19. This needs a "Returns" section.

  20. This needs a "Returns" section.

  21. 
      
matthewgoodman13
Review request changed

Change Summary:

David's comments + changes to how emoji index is retrieved for optimization

Commit:

-addf68d15814eef3b3c01c4b7fc3d6ca463171ec
+757311866922ce1b6962f4cf40a35a985bf59127

Diff:

Revision 4 (+4217)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

matthewgoodman13
Review request changed

Summary:

-[WIP | Emoji UI Picker] [PART 2 | Djblets] Working implementation ready for review
+[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation

Description:

~  

[WIP | Emoji UI Picker] [PART 2 | Djblets] Working implementation ready for review

  ~

[Emoji UI Picker] [PART 2 | Djblets] Finished working implementation

   
~  

=> Would like a preliminary review

  ~

See Final Report

-   => Still need to add unit tests

   
   

Image

Testing Done:

   

=> Local testing

~   => Unit tests needed

  ~ => Unit tests added for emoji registry methods

Commit:

-a5f50f622be37879c7a32741a7889bb57db02cdd
+a864bc5f85b755534984dffe5f122710f64d4432

Diff:

Revision 9 (+4386)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Loading...