-
Notifications
You must be signed in to change notification settings - Fork 753
Use importlib_metadata for 3.8 and 3.9 only #4177
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
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,18 +12,92 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# FIXME: Use importlib.metadata when support for 3.11 is dropped if the rest of | ||
# the supported versions at that time have the same API. | ||
from importlib_metadata import ( # type: ignore | ||
EntryPoint, | ||
EntryPoints, | ||
entry_points, | ||
version, | ||
) | ||
|
||
# The importlib-metadata library has introduced breaking changes before to its | ||
# API, this module is kept just to act as a layer between the | ||
# importlib-metadata library and our project if in any case it is necessary to | ||
# do so. | ||
|
||
__all__ = ["entry_points", "version", "EntryPoint", "EntryPoints"] | ||
from sys import version_info | ||
|
||
if version_info < (3, 10): | ||
|
||
# pylint: disable=import-error | ||
from importlib_metadata import version # type: ignore | ||
from importlib_metadata import ( | ||
Distribution, | ||
EntryPoint, | ||
EntryPoints, | ||
PackageNotFoundError, | ||
distributions, | ||
) | ||
from importlib_metadata import ( | ||
entry_points as importlib_metadata_entry_points, | ||
) | ||
from importlib_metadata import requires | ||
|
||
else: | ||
|
||
from importlib.metadata import ( # type: ignore | ||
Distribution, | ||
EntryPoint, | ||
EntryPoints, | ||
PackageNotFoundError, | ||
distributions, | ||
) | ||
from importlib.metadata import ( | ||
entry_points as importlib_metadata_entry_points, | ||
) | ||
|
||
from importlib.metadata import requires, version # isort: skip | ||
|
||
|
||
def entry_points(**params) -> EntryPoints: # type: ignore | ||
""" | ||
Same as entry_points but requires at least one argument | ||
For Python 3.8 or 3.9: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there was a proposal for a unified solution for the |
||
isinstance( | ||
importlib_metadata.entry_points(), importlib_metadata.EntryPoints | ||
) | ||
evaluates to True. | ||
For Python 3.10, 3.11: | ||
isinstance( | ||
importlib.metadata.entry_points(), importlib.metadata.SelectableGroups | ||
) | ||
evaluates to True. | ||
For Python 3.12: | ||
isinstance( | ||
importlib.metadata.entry_points(), importlib.metadata.EntryPoints | ||
) | ||
evaluates to True. | ||
So, when called with no arguments, entry_points returns objects of | ||
different types depending on the Python version that is being used. This is | ||
obviously very problematic. Nevertheless, in our code we don't ever call | ||
entry_points without arguments, so the approach here is to redefine | ||
entry_points so that it requires at least one argument. | ||
""" | ||
|
||
if not params: # type: ignore | ||
raise ValueError("entry_points requires at least one argument") | ||
return importlib_metadata_entry_points(**params) # type: ignore | ||
|
||
|
||
__all__ = [ | ||
"entry_points", | ||
"version", | ||
"EntryPoint", | ||
"EntryPoints", | ||
"requires", | ||
"Distribution", | ||
"distributions", | ||
"PackageNotFoundError", | ||
] |
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
asgiref==3.7.2 | ||
Deprecated==1.2.14 | ||
iniconfig==2.0.0 | ||
packaging==24.0 | ||
pluggy==1.5.0 | ||
py-cpuinfo==9.0.0 | ||
pytest==7.4.4 | ||
tomli==2.0.1 | ||
typing_extensions==4.10.0 | ||
wrapt==1.16.0 | ||
zipp==3.19.2 | ||
-e opentelemetry-sdk | ||
-e opentelemetry-semantic-conventions | ||
-e tests/opentelemetry-test-utils | ||
-e opentelemetry-api |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of not using
importlib_metadata
for newer Python versions?I think this branch alone would solve all of our problems and you wouldn't need the custom
def entry_points()
since importlib_metadata has already provided a python version agnostic API for usThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed something, but I see the main benefit as simply relying on a built-in module instead of external dependency, given that
importlib.metadata
is no longer provisional since Python 3.10 and we have a stable API. Also minimize issues related to the pinned version ofimportlib-metadata
packageUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely see your point, Emidio, but my vote is for the simpler solution here until
importlib_metadata
is no longer needed. Seems like the cost is quite small.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on only relying on one library throughout the codebase. I am FOR using
importlib_metadata
for now for the reasons @aabmass pointed out in terms of consistency. I am concerned that future contributions will not always use this library and use the standard built-in one instead, after which we must always remember as reviewers that we still support Python versions in which that will not be possible to do. This might be a calculated risk, as the contributions related to these libraries are relatively small, so if the vast majority of us agrees on that and the benefits of using a single, simple library out weigh the review/mental overhead of forcing users to use the backport then we can go ahead with this suggestion.Keep in mind that if we do somehow want to go with the "custom built in entrypoint()" function of the current PR as of writing this review, our message to contributors in the future will be "use this custom thing we built", which technically is amounts to the same amount of headache/overhead as the previous problem.