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

Fix: Replace setuptools.extern.packaging with direct packaging import #913

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

WassCodeur
Copy link
Member

Description

This PR corrects a problem related to the use of setuptools.extern.packaging, which is not standard and not recommended by setuptools. The problem occurs with versions of setuptools >= 71.0.3, causing an ImportError when deprecated components are called.

Changes

  • Replacement of setuptools.extern.packaging by direct import of packaging into deprecator.py.

Reason

The setuptools.extern module was removed in >=71.0.0 versions of setuptools, causing import errors. By updating the import declaration to use packaging, we ensure that FURY remains compatible.

Tests

  • Run all existing tests to ensure no regression.
  • Checked depreciation warning and error handling logic.

Fix issue #912

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @WassCodeur , thank you for this update. See below my comment

@@ -19,7 +19,7 @@
# packaging.version.parse is a third-party utility but is used by setuptools
# (so probably already installed) and is conformant to the current PEP 440.
# But just if it is not the case, we use distutils
packaging, have_pkg, _ = optional_package("setuptools.extern.packaging")
packaging, have_pkg, _ = optional_package("packaging")
Copy link
Contributor

Choose a reason for hiding this comment

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

Packaging is a mandatory package so you do not need optional_package function and you can import it directly.

Thank you for the future update!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @skoudoro

Thank you for your feedback. I'll take it into account in my next update.

     - update deprecator.py to use packaging instead of optional_package
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

LGTM, merging, Thanks @WassCodeur

@skoudoro skoudoro merged commit e7932d6 into fury-gl:master Jul 24, 2024
17 of 29 checks passed
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