carpedm20 / emoji

emoji terminal output for Python
Other
1.91k stars 278 forks source link

Update core.py #303

Closed Aaryanverma closed 2 months ago

Aaryanverma commented 2 months ago

import Literal, Match and TypedDict from "typing" instead of "typing_extensions"

cvzi commented 2 months ago

Please enable Github actions in your repository and run the CI tests. I am pretty sure this fails on Python 3.7

Aaryanverma commented 2 months ago

yeah, actually this is for python >= 3.10 because it is failing there. Can you update python version to at least 3.9 for this one.

cvzi commented 2 months ago

Is there a problem at the moment with Python 3.10 or is this just to simplify the code?

Aaryanverma commented 2 months ago

yes with python 3.10, it is throwing error that "cannot import Match from typing_extensions". importing from "typing" should fix the issue. (using latest version of typing_extensions)

cvzi commented 2 months ago

I can't reproduce this on Python 3.10 with latest typing-extensions==4.12.2.

There was a similar issue raised previously: #301 They solved it:

I noticed this problem was raised because of a need of restart of my databricks notebook.

Aaryanverma commented 2 months ago

For me it's not getting resolved after restarting databricks notebook. I created this PR using the following issue reference: https://github.com/carpedm20/emoji/issues/301#issuecomment-2331428130

cvzi commented 2 months ago

The typing_extensions source code has Literal, Match and TypedDict listed, so importing them should work: https://github.com/python/typing_extensions/blob/4.12.2/src/typing_extensions.py#L13-L134

Could you check that typing_extensions is actually installed correctly? The following should print the same list as the source code above and the installed version of typing_extensions

import typing_extensions
print(typing_extensions.__all__)
import importlib.metadata
print(importlib.metadata.version('typing-extensions'))
Aaryanverma commented 2 months ago

python==3.10 typing_extensions==4.12.2 emoji==2.12.1 databricks environment

still getting error: cannot import name Match from typing_extensions

cvzi commented 2 months ago

I think this is a bug in databricks then, maybe open a bug report with them.

cvzi commented 2 months ago

Maybe it is possible to remove the dependency for typing_extensions in newer Python versions and only include it in the older Python versions that need it. Then the import could be changed to

if is_old_python_version:
    from typing_extensions import Literal, Match, TypedDict
else:
    from typing import Literal, Match, TypedDict

I'll look into it.


Dropping older Python version is not an option, there are still too many people using them. Download stats for last month on pypi.org for emoji package: python percent downloads
3.8 37.13% 2,137,043
3.10 23.36% 1,344,485
3.11 16.92% 973,940
null 5.86% 337,090
3.7 5.53% 318,390
3.12 5.32% 305,902
3.9 5.19% 298,541
3.6 0.42% 23,912
2.7 0.24% 13,699
3.5 0.03% 1,676
3.13 0.01% 382
3.4 0.00% 14
3.14 0.00% 4
Total 5,755,078

Date range: 2024-08-01 - 2024-08-31

Aaryanverma commented 2 months ago

Maybe it is possible to remove the dependency for typing_extensions in newer Python versions and only include it in the older Python versions that need it.

if this is possible, this will be really helpful. Let me know when you can implement this, I kind of need this at the earliest for my project. Let me know if I can help somewhere.

cvzi commented 2 months ago

typing_extensions is also imported in the files tests/test_core.py and in utils/testutils.py. Could you make similar changes there as well?

I'll test it today or tomorrow and then it can be released.

Aaryanverma commented 2 months ago

abovesaid changes are done, waiting for testing and release

cvzi commented 2 months ago

@Aaryanverma Thank you!

Something went wrong with this pull request, not sure what exactly, you can just close this pull request. I have merged your changes into a new pull request: #307

Aaryanverma commented 2 months ago

Closing this PR as abovesaid changes are merged in this one: https://github.com/carpedm20/emoji/pull/307