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

pkg_resources: Expose the type of imports and re-exports #4242

Closed
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 68 additions & 56 deletions pkg_resources/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
import time
import re
import types
from typing import List, Protocol
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Protocol
from collections.abc import Callable, Iterator
import zipfile
import zipimport
import warnings
Expand Down Expand Up @@ -65,40 +66,44 @@
from os import open as os_open
from os.path import isdir, split

from pkg_resources.extern.jaraco.text import (
yield_lines,
drop_comment,
join_continuation,
)
if not TYPE_CHECKING:
Avasam marked this conversation as resolved.
Show resolved Hide resolved
from pkg_resources.extern.jaraco.text import (
yield_lines,
drop_comment,
join_continuation,
)

from pkg_resources.extern import platformdirs
from pkg_resources.extern import packaging

__import__('pkg_resources.extern.packaging.version')
__import__('pkg_resources.extern.packaging.specifiers')
__import__('pkg_resources.extern.packaging.requirements')
__import__('pkg_resources.extern.packaging.markers')
__import__('pkg_resources.extern.packaging.utils')

# declare some globals that will be defined later to
# satisfy the linters.
require = None
working_set = None
add_activation_listener = None
resources_stream = None
cleanup_resources = None
resource_dir = None
resource_stream = None
set_extraction_path = None
resource_isdir = None
resource_string = None
iter_entry_points = None
resource_listdir = None
resource_filename = None
resource_exists = None
_distribution_finders = None
_namespace_handlers = None
_namespace_packages = None
from pkg_resources.extern import platformdirs
from pkg_resources.extern import packaging

__import__('pkg_resources.extern.packaging.version')
__import__('pkg_resources.extern.packaging.specifiers')
__import__('pkg_resources.extern.packaging.requirements')
__import__('pkg_resources.extern.packaging.markers')
__import__('pkg_resources.extern.packaging.utils')
else:
# Replicates the imports above in a way that can be understood by type-checkers
from jaraco.text import (
yield_lines,
drop_comment,
join_continuation,
)
import platformdirs
import packaging
import packaging.version
import packaging.specifiers
import packaging.requirements
import packaging.markers
import packaging.utils
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit tricky...

Based on the comments:

I suppose this would be a controversial change (I am not sure if it matters to have the if TYPE_CHECKING in pkg_resources/__init__.py or pkg_resouces/extern/__init__.py, probably the comments apply to both).

At the end of the day, mypy cannot handle MetaPathFinders... I suppose that the ultimate solution would be to use a mypy plugin to "convince" mypy to use the files in the */_vendor folder for the *.extern modules, but that sounds very complicated1. So there is a trade-off here: complexity of implementing a mypy plugin vs. in the duplication in *.extern modules vs. leave the things unchecked (and loosing some of the benefits of the type analysis).

@jaraco, do you have any opinion about this one?

Footnotes

  1. I cannot find a hook for finding modules in the docs: https://mypy.readthedocs.io/en/stable/extending_mypy.html#extending-mypy-using-plugins. So I opened https://github.com/python/mypy/issues/16988 for guidance on this.

Copy link
Contributor Author

@Avasam Avasam Mar 5, 2024

Choose a reason for hiding this comment

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

It's slightly different from what I originally did in #3979 in that it's only duplicating what's needed/used. And doing so right next to the equivalent dynamic imports. So imo there's less of a maintenance concern.


packaging.requirements is especially important to avoid looking like we're subclassing Any for RequirementParseError and Requirement

With static imports:
image

W/o the import behind a TYPE_CHECKING:
image


FWIW, I'd rather loose on type safety internally, publicly loose on implicit return types (until all public API return types are explicit), and loose on some super class typing, than going for a mypy plugin. Which also only works on mypy, not pyright, VSCode+Pylance, or PyCharm.

I'll probably end up keeping those changes in my local working branches since it helps me validate the types and understand intended behaviour. Even if it doesn't end up in main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hang on, now that there's a cog script/template thingy, could I extend it to do #3979 (comment) again but automated this time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't see why not. But currently there is a problem with the script that I am fixing in #4353, so it might make sense to wait for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I will defer the final decision about any workarounds for the lack of support of MetaPathFinders in type checkers to Jason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm likely gonna do this last anyway. It's just good to know I have more options to try and suggest.


# Declare some globals that will be defined by calls to _declare_state so checkers know they exist and their type
_distribution_finders: Dict[
Avasam marked this conversation as resolved.
Show resolved Hide resolved
type, Callable[[Any, str, bool], Iterator["Distribution"]]
] = {}
_namespace_handlers: Dict[
type, Callable[[Any, str, str, types.ModuleType], Optional[str]]
] = {}
_namespace_packages: Dict[Optional[str], List[str]] = {}


warnings.warn(
Expand Down Expand Up @@ -491,19 +496,6 @@ def compatible_platforms(provided, required):
return False


def run_script(dist_spec, script_name):
"""Locate distribution `dist_spec` and run its `script_name` script"""
ns = sys._getframe(1).f_globals
name = ns['__name__']
ns.clear()
ns['__name__'] = name
require(dist_spec)[0].run_script(script_name, ns)
Avasam marked this conversation as resolved.
Show resolved Hide resolved


# backward compatibility
run_main = run_script


def get_distribution(dist):
"""Return a current distribution object for a Requirement or string"""
if isinstance(dist, str):
Expand Down Expand Up @@ -3276,6 +3268,15 @@ def _mkstemp(*args, **kw):
warnings.filterwarnings("ignore", category=PEP440Warning, append=True)


class PkgResourcesDeprecationWarning(Warning):
"""
Base class for warning about deprecations in ``pkg_resources``

This class is not derived from ``DeprecationWarning``, and as such is
visible by default.
"""


# from jaraco.functools 1.3
def _call_aside(f, *args, **kwargs):
f(*args, **kwargs)
Expand All @@ -3294,15 +3295,6 @@ def _initialize(g=globals()):
)


class PkgResourcesDeprecationWarning(Warning):
"""
Base class for warning about deprecations in ``pkg_resources``

This class is not derived from ``DeprecationWarning``, and as such is
visible by default.
"""


@_call_aside
def _initialize_master_working_set():
"""
Expand Down Expand Up @@ -3338,3 +3330,23 @@ def _initialize_master_working_set():
# match order
list(map(working_set.add_entry, sys.path))
globals().update(locals())


if TYPE_CHECKING:
# All of these are set by the @_call_aside methods above
__resource_manager: ResourceManager = ... # Won't exist at runtime
resource_exists = __resource_manager.resource_exists
resource_isdir = __resource_manager.resource_isdir
resource_filename = __resource_manager.resource_filename
resource_stream = __resource_manager.resource_stream
resource_string = __resource_manager.resource_string
resource_listdir = __resource_manager.resource_listdir
set_extraction_path = __resource_manager.set_extraction_path
cleanup_resources = __resource_manager.cleanup_resources

working_set: WorkingSet = ...
require = working_set.require
iter_entry_points = working_set.iter_entry_points
add_activation_listener = working_set.subscribe
run_script = working_set.run_script
run_main = run_script
Loading