Skip to content
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

Update core.py #303

Closed
wants to merge 3 commits into from
Closed

Update core.py #303

wants to merge 3 commits into from

Conversation

Aaryanverma
Copy link
Contributor

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

import Literal, Match and TypedDicr from "typing" instead of "typing_extensions"
@cvzi
Copy link
Contributor

cvzi commented Sep 19, 2024

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

@Aaryanverma
Copy link
Contributor Author

Aaryanverma commented Sep 19, 2024

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
Copy link
Contributor

cvzi commented Sep 19, 2024

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

@Aaryanverma
Copy link
Contributor Author

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
Copy link
Contributor

cvzi commented Sep 19, 2024

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
Copy link
Contributor Author

Aaryanverma commented Sep 19, 2024

For me it's not getting resolved after restarting databricks notebook. I created this PR using the following issue reference:
#301 (comment)

@cvzi
Copy link
Contributor

cvzi commented Sep 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

cvzi commented Sep 19, 2024

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

@cvzi
Copy link
Contributor

cvzi commented Sep 20, 2024

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
Copy link
Contributor Author

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.

check for python version if less than 3.9 then import from typing_extension else import from typing
@cvzi
Copy link
Contributor

cvzi commented Sep 21, 2024

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
Copy link
Contributor Author

abovesaid changes are done, waiting for testing and release

@cvzi
Copy link
Contributor

cvzi commented Sep 21, 2024

@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
Copy link
Contributor Author

Closing this PR as abovesaid changes are merged in this one: #307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants