-
Notifications
You must be signed in to change notification settings - Fork 277
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
Code enhancements suggestions #292
Comments
Sounds great. I would generally be open to any of those. Moving the EMOJI_DATA to json files with a separate json file for each language ( #280 ) would be a breaking change, so that should be a major version change anyway.
I am not familiar with 4 a/b Sorry, I don't have much time at the moment. |
|
I'll look into 4 b: That should be possible without breaking changes, since |
While adding type hints to the library i found various places where in my opinion improvments could be made.
I add them here to discuss because most of them will be breaking changes, and should only be considered once a major revision is planned.
1)
This method returns 2 completely different dicts, i don’t think its evident for the user what they will get here, and if the user wants to prevent bugs, even with type hints, after each call the user needs to test which kind of dict he got.
I got a branch where all dicts are typed, but this is still a pain here.
Also
deepcopy
should be considered here, because the emoji data contain itself mutable objects again.2)
The user wants to call
token.value.emoji
but he first needs to test what he got withisinstance(token.value, str)
3)
In
data_dict.py
:STATUS
should be anIntEnum
, i found no reason why this is a dict.4)
a)
Instead of returning mutable dicts, we could return a
MappingProxy
Object which wraps the dict, this makes it (as much as you can in python) immutable. And the user cannot accidentally modify it.b)
Instead of the constant
_EMOJI_UNICODE
and_ALIASES_UNICODE
one could use@functools.lru_cache
which would make the code much smaller.5)
A lot of test code imports the private cache
_EMOJI_UNICODE
. While this is probably not really a problem it makes it so we have to add manytype: ignore
comments.A simple helper method in the test module which iterates the languages and builds the whole cache would let us get rid of all the comments.
The text was updated successfully, but these errors were encountered: