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

Dont rely on the existence of "select" on entry points #1000

Merged
merged 2 commits into from
Jul 18, 2021
Merged

Conversation

rahulporuri
Copy link
Contributor

This PR adds a compatibility layer to the entry point code to handle the absence of the select method on entry points. Additionally, we now rely on the external importlib_metadata first and only if that is absent do we rely on importlib.metadata from the standard library.

fixes #998

Note to reviewer : I tested this locally by creating a python 3.9 virtual environment, installing pyface and relevent test dependencies and running the testsuite. On this branch, I don't see the select-related test errors that we see on the master branch.

This commit adds a compatibility layer to the entry point code to handle
the absence of the select method on entry points. Additionally, we now
rely on the external importlib_metadata first and only if that is absent
do we rely on importlib.metadata from the standard library

	modified:   pyface/base_toolkit.py
	modified:   pyface/tests/test_toolkit.py
@rahulporuri
Copy link
Contributor Author

@mdickinson do you have the bandwidth to review?

@@ -68,11 +68,14 @@
import sys

try:
import importlib_metadata
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer something that defaults to the std. lib. functionality where it's available and only uses importlib_metadata as a fallback, since the std. lib. is really what we want to be using: we don't want to be relying on the 3rd party version for any longer than we have to.

With the changes below, we don't actually need to do this reversal, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with these changes, we don't need this reversal but I still chose the reversal specifically we made importlib_metadata a required dependency for Python < 3.10 -

'importlib-metadata>=3.6.0; python_version<"3.10"',

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but I'd prefer to get rid of the importlib_metadata dependency as soon as we can - i.e., with Python 3.8.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM as a quick fix, but I'd rather have a solution that preferred importlib.metadata over importlib_metadata. That way we can get rid of the 3rd party dependency as soon as we can require Python >= 3.8, and we don't have to wait for Python 3.10.

and limit the dependency on external importlib_metadata to python < 3.8

	modified:   pyface/__init__.py
	modified:   pyface/base_toolkit.py
	modified:   pyface/tests/test_toolkit.py
@rahulporuri
Copy link
Contributor Author

@mdickinson can you re-review this PR? I've removed the import reversal - and removed the dependence on external importlib_metadata for python >= 3.8

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for the update.

Tested in venvs for Python 3.6, 3.7, 3.8, 3.9 and 3.10(beta), though not surprisingly I wasn't able to get the test suite to run well in 3.10.

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.

Toolkit selection is broken on Python 3.9
2 participants