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

Remove typing_extensions from the tests requirements #1944

Merged
merged 4 commits into from
Jan 8, 2023

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jan 8, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix
βœ“ ✨ New feature
βœ“ πŸ”¨ Refactoring
βœ“ πŸ“œ Docs

Description

The tests had their very own version of typing extensions which prevented us from seeing it was a run time dependencies and made the tests useless to actually see this issue. See #1942 (comment)

Closes #1945

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.2 milestone Jan 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Jan 8, 2023
@codecov
Copy link

codecov bot commented Jan 8, 2023

Codecov Report

Merging #1944 (c43bc39) into main (6d936e8) will not change coverage.
The diff coverage is n/a.

❗ Current head c43bc39 differs from pull request most recent head c6e3667. Consider uploading reports for the commit c6e3667 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1944   +/-   ##
=======================================
  Coverage   92.63%   92.63%           
=======================================
  Files          94       94           
  Lines       10869    10869           
=======================================
  Hits        10069    10069           
  Misses        800      800           
Flag Coverage Ξ”
linux 92.40% <ΓΈ> (ΓΈ)
pypy 88.54% <ΓΈ> (ΓΈ)
windows 92.31% <ΓΈ> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the fix-typing-extensions-requirements branch from 2159af3 to f6b3495 Compare January 8, 2023 12:44
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jan 8, 2023

f6b3495 's CI should fail for python 3.10.

ChangeLog Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas merged commit 19878a5 into main Jan 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas deleted the fix-typing-extensions-requirements branch January 8, 2023 13:11
github-actions bot pushed a commit that referenced this pull request Jan 8, 2023
Also fix the version check for 'typing-extensions' dependency in order to fix #1945

(cherry picked from commit 19878a5)
Pierre-Sassoulas added a commit that referenced this pull request Jan 8, 2023
Also fix the version check for 'typing-extensions' dependency in order to fix #1945

(cherry picked from commit 19878a5)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Fokko added a commit to Fokko/iceberg that referenced this pull request Jan 8, 2023
It is an issue in a downstream dependency, and is currently being
fixed in pylint: pylint-dev/pylint#8030

Has been fixed in Astroid in:
pylint-dev/astroid#1944
Fokko added a commit to Fokko/iceberg that referenced this pull request Jan 8, 2023
It is an issue in a downstream dependency, and is currently being
fixed in pylint: pylint-dev/pylint#8030

Has been fixed in Astroid in:
pylint-dev/astroid#1944
rdblue pushed a commit to apache/iceberg that referenced this pull request Jan 8, 2023
It is an issue in a downstream dependency, and is currently being
fixed in pylint: pylint-dev/pylint#8030

Has been fixed in Astroid in:
pylint-dev/astroid#1944
"typing-extensions>=4.0.0;python_version<'3.10'",
"typing-extensions>=4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? From what I can see, typing-extensions are only required on python versions lower than 3.11 (because typing.Self is new in 3.11) and 3.13 or newer (because TypedDict will be removed from typing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically this is correct though. We should only need this on <3.11. However, it is easier to not have to update this and just install every time.

Are you in an environment where typing-extensions is unavailable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to fix these issues in #1964 and tests pass for me locally without typing_extensions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is correct though. We should only need this on <3.11. However, it is easier to not have to update this and just install every time.

Are you in an environment where typing-extensions is unavailable?

I just noticed a new typing-extensions dependency that seemed to be only necessary for tests and after looking a bit into it, it seems like it's not required for tests either with a small modification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll have a look! Thanks!

@cdce8p cdce8p added the backported Assigned once the backport is done label Jan 31, 2023
@cdce8p cdce8p modified the milestone: 2.13.2 Jan 31, 2023
Fokko added a commit to apache/iceberg-python that referenced this pull request Sep 29, 2023
It is an issue in a downstream dependency, and is currently being
fixed in pylint: pylint-dev/pylint#8030

Has been fixed in Astroid in:
pylint-dev/astroid#1944
Fokko added a commit to apache/iceberg-python that referenced this pull request Sep 30, 2023
It is an issue in a downstream dependency, and is currently being
fixed in pylint: pylint-dev/pylint#8030

Has been fixed in Astroid in:
pylint-dev/astroid#1944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Assigned once the backport is done Bug πŸͺ³ Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModuleNotFoundError: No module named 'typing_extensions'
4 participants