Skip to content

gh-121604: Raise DeprecationWarnings in importlib #121765

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.

Sign up for GitHub

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
wants to merge 7 commits into from
Closed

gh-121604: Raise DeprecationWarnings in importlib #121765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03bbac0
Add deprecation warnings
rashansmith Jul 14, 2024
820ac21
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 14, 2024
d6c0645
Update Lib/importlib/_abc.py
rashansmith Jul 14, 2024
cf8264d
Apply suggestions from code review
rashansmith Jul 14, 2024
cfbe079
Update machinery deprecation warning
rashansmith Jul 14, 2024
6294458
Apply suggestions from code review
rashansmith Jul 15, 2024
f2458cb
Merge branch 'main' into main
brettcannon Sep 26, 2024
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
5 changes: 5 additions & 0 deletions Lib/importlib/_abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
"""Subset of importlib.abc used to reduce importlib.util imports."""
from . import _bootstrap
import abc
import warnings


class Loader(metaclass=abc.ABCMeta):

"""Abstract base class for import loaders."""
def __init__(self):
warnings.warn(f"Loader is deprecated.",
DeprecationWarning, stacklevel=2)
super().__init__()
Comment on lines +10 to +13
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure but I think we should raise the deprecation warning when the class is imported, not when you create a new instance with it. Going by the example in the original PEP 562 it'd look (I think) something like this:

# Rename the deprecated class
class _Loader(metaclass=abc.ABCMeta):
    ...

def __getattr__(name):
    if name == 'Loader':
        warnings.warn(f"The 'Loader' class is deprecated.",
                      DeprecationWarning, stacklevel=2)
        return _Loader
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, no problem. Thanks for the reference link! Another approach I had was to just put the warning message by itself in the first line under the class name. Would this have a similar affect? I removed it because I then saw the deprecation error when running the make command.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem with the other approach is that you'll get the warning whenever you import the module, not just the deprecated class specifically. For example, import importlib.abc will produce a warning for InspectLoader even if you're not actually using it


def create_module(self, spec):
"""Return a module to initialize and into which to load.
Expand Down
11 changes: 11 additions & 0 deletions Lib/importlib/_bootstrap_external.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,10 @@ def path_mtime(self, path):

Raises OSError when the path cannot be handled.
"""
_warnings.warn(
f"SourcelessFileLoader is deprecated.",
DeprecationWarning,
stacklevel=2)
raise OSError

def path_stats(self, path):
Expand Down Expand Up @@ -1000,6 +1004,13 @@ def set_data(self, path, data, *, _mode=0o666):
class SourcelessFileLoader(FileLoader, _LoaderBasics):

"""Loader which handles sourceless file imports."""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
_warnings.warn(
f"SourcelessFileLoader is deprecated.",
DeprecationWarning,
stacklevel=2
)

def get_code(self, fullname):
path = self.get_filename(fullname)
Expand Down
13 changes: 13 additions & 0 deletions Lib/importlib/abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
_frozen_importlib_external = _bootstrap_external
from ._abc import Loader
import abc
import warnings


__all__ = [
Expand All @@ -22,6 +23,11 @@
]


def __getattr__(name):
if name == 'ResourceLoader':
warnings.warn(f"The '{name}' attribute is deprecated.",
DeprecationWarning, stacklevel=2)

def _register(abstract_cls, *classes):
for cls in classes:
abstract_cls.register(cls)
Expand Down Expand Up @@ -69,6 +75,8 @@ class ResourceLoader(Loader):
This ABC represents one of the optional protocols specified by PEP 302.

"""
warnings.warn(f"The Resource Loader class is deprecated.",
DeprecationWarning, stacklevel=2)

@abc.abstractmethod
def get_data(self, path):
Expand All @@ -85,6 +93,8 @@ class InspectLoader(Loader):
This ABC represents one of the optional protocols specified by PEP 302.

"""
warnings.warn(f"The InspectLoader class is deprecated.",
DeprecationWarning, stacklevel=2)

def is_package(self, fullname):
"""Optional method which when implemented should return whether the
Expand Down Expand Up @@ -198,6 +208,9 @@ class SourceLoader(_bootstrap_external.SourceLoader, ResourceLoader, ExecutionLo
"""

def path_mtime(self, path):
"""Deprecated"""
warnings.warn(f"The path_mtime function is deprecated.",
DeprecationWarning, stacklevel=2)
"""Return the (int) modification time for the path (str)."""
if self.path_stats.__func__ is SourceLoader.path_stats:
raise OSError
Expand Down
13 changes: 13 additions & 0 deletions Lib/importlib/machinery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""The machinery of importlib: finders, loaders, hooks, etc."""

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 keep the newline

import warnings
from ._bootstrap import ModuleSpec
from ._bootstrap import BuiltinImporter
from ._bootstrap import FrozenImporter
Expand Down Expand Up @@ -27,3 +28,15 @@ def all_suffixes():
'NamespaceLoader', 'OPTIMIZED_BYTECODE_SUFFIXES', 'PathFinder',
'SOURCE_SUFFIXES', 'SourceFileLoader', 'SourcelessFileLoader',
'WindowsRegistryFinder', 'all_suffixes']


def __getattr__(name):
if name in ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES', 'WindowsRegistryFinder'):
Copy link
Contributor

Choose a reason for hiding this comment

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

The first if statement could be omitted, just use the if-elif statement ;)

if name in ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES'):
    ...
elif name == 'WindowsRegistryFinder':
    ...

if name in ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES'):
warnings.warn(f"The '{name}' module is deprecated.",
DeprecationWarning, stacklevel=2)
return name
else:
warnings.warn(f"The '{name}' class is deprecated.",
DeprecationWarning, stacklevel=2)
return name
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-07-14-12-46-02.gh-issue-121754.2Tt2fE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This PR adds some deprecation warnings to importlib
Loading