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

reqs: add typing-extension dependency #1263

Merged
merged 6 commits into from Jul 14, 2023
Merged

reqs: add typing-extension dependency #1263

merged 6 commits into from Jul 14, 2023

Conversation

superlopuh
Copy link
Member

@superlopuh superlopuh commented Jul 12, 2023

Self is a very useful type to have available, introduced in 3.11. In Python 3.10 a third-party module is required for the same functionality. I reckon the extra dependency is worth it for the kind of thing we'd like to express.

It seems to me like it's necessary to make it a required dependency as core code should be able to use Self, so users of pip install should inherit it as well. It's a bit annoying to have a dependency that's only required for python 3.10 but it seems worth it to me.

@superlopuh superlopuh added the xdsl xdsl framework specific changes label Jul 12, 2023
@superlopuh superlopuh self-assigned this Jul 12, 2023
@superlopuh superlopuh added dependencies Pull requests that update a dependency file and removed xdsl xdsl framework specific changes labels Jul 12, 2023
@superlopuh superlopuh changed the title [WIP] misc: add Self type reqs: add typing-extensions dependency Jul 12, 2023
@superlopuh superlopuh changed the title reqs: add typing-extensions dependency reqs: add typing-extensions optional dependency Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (df8d4c8) 89.10% compared to head (045a22c) 89.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1263      +/-   ##
==========================================
+ Coverage   89.10%   89.12%   +0.01%     
==========================================
  Files         181      181              
  Lines       24124    24075      -49     
  Branches     3667     3668       +1     
==========================================
- Hits        21496    21457      -39     
+ Misses       2048     2040       -8     
+ Partials      580      578       -2     
Impacted Files Coverage Δ
xdsl/interpreters/shaped_array.py 98.14% <100.00%> (+0.03%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@superlopuh superlopuh changed the title reqs: add typing-extensions optional dependency reqs: add typing-extension dependency Jul 12, 2023
@superlopuh superlopuh marked this pull request as ready for review July 12, 2023 17:30
Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think when will python 3.11 be shipped with ubuntu? I guess at that point we could remove 3.10 in general, IMO.

Until then, I am fine with having this additional dependency.

@superlopuh
Copy link
Member Author

Yeah ideally we'd drop it as soon as we drop 3.10

@kingiler
Copy link
Collaborator

@webmiche Since Ubuntu Lunar (23.04), python3 is 3.11 as in here.

Copy link
Collaborator

@kingiler kingiler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

requirements.txt Outdated
@@ -1,2 +1,3 @@
pip<24.0
immutabledict<2.2.6
typing-extensions<4.8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
typing-extensions<4.8
typing-extensions>=4.8,<5

as suggested on the homepage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a go, although it might not work with some of the dependency tracking tools, will report back

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked! I was too pessimistic. Let's see how dependabot deals with the update

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
I don't think we want to remove Python 3.10 support any time soon though (if we can easily keep compatibility, we should). But I think that depending on typing-extensions is quite safe!

@superlopuh superlopuh requested a review from kingiler July 13, 2023 18:36
@superlopuh superlopuh merged commit 03b2de6 into main Jul 14, 2023
10 checks passed
@superlopuh superlopuh deleted the sasha/self-type branch July 14, 2023 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants