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.

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

Jump to bottom

Add Distribution.from_name arg validation #389

Closed
wants to merge 3 commits into from
Closed

Add Distribution.from_name arg validation #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4ae953
Add xfail test capturing new expectation.
jaraco Jun 23, 2022
71cec30
Add a validator to Distribution.from_name
jaraco Jun 23, 2022
fdbcb79
Add pydantic as a build dependency as possible workaround for failing…
jaraco Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion importlib_metadata/__init__.py
View file
Open in desktop
Expand Up @@ -29,6 +29,8 @@
from importlib import import_module
from importlib.abc import MetaPathFinder
from itertools import starmap
from pydantic import Field, validate_arguments
from pydantic.typing import Annotated
jaraco marked this conversation as resolved.
Show resolved Hide resolved
from typing import List, Mapping, Optional, Union


Expand Down Expand Up @@ -548,7 +550,8 @@ def locate_file(self, path):
"""

@classmethod
def from_name(cls, name):
@validate_arguments
def from_name(cls, name: Annotated[str, Field(min_length=1)]):
Copy link
Member

Choose a reason for hiding this comment

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

Why are we introducing a dependency here? Won't we have to port this to CPython?

Also, personally, I'm unconvinced in introducing a dependency for such a simple check 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Over a year ago, I presented at the Python Language Summit a proposal to use annotations to provide validators and transformers. The recommendation then was for me to explore pydantic, so I'm doing that now. I'm not certain I want to add a dependency, but I also want to explore the space. If it works well, I'll want to expand its usage to other similar patterns. As for porting to CPython, it's my understanding that parts of pydantic are being ported to CPython. I'm not sure if these parts are or not, but if not, this and related use-cases can help inform a rationale why they should.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's something particularly elegant about this approach. In addition to not requiring explicit code in the body to validate the parameter, it also offloads from the author the concern about the correct error to raise here. Instead both the author and the consumer can assume that the error will be consistent with similar parameter validations. It also opens up options for optimizing performance by validating checks statically and disabling validations at runtime.

I still need to explore the viability of adding a dependency, especially in light of CPython integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

As much as I'd like to explore the use of pydantic, I'm now realizing that the cost of adding a dependency here is very high, not only because of the CPython integration but also because of the race condition with setuptools_scm. I'm going to abandon this approach in favor of a manual check in #391. I would eventually like to explore this concept again, but today is not the day.

"""Return the Distribution for the given package name.

:param name: The name of the distribution package to search for.
Expand Down
8 changes: 7 additions & 1 deletion pyproject.toml
View file
Open in desktop
@@ -1,5 +1,11 @@
[build-system]
requires = ["setuptools>=56", "setuptools_scm[toml]>=3.4.1"]
requires = [
"setuptools>=56",
"setuptools_scm[toml]>=3.4.1",
# temporary workaround for dependency race in setuptools_scm
# https://github.com/python/importlib_metadata/pull/389#issuecomment-1164912426
'pydantic; python_version < "3.8"',
]
build-backend = "setuptools.build_meta"

[tool.black]
Expand Down
1 change: 1 addition & 0 deletions setup.cfg
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ python_requires = >=3.7
install_requires =
zipp>=0.5
typing-extensions>=3.6.4; python_version < "3.8"
pydantic

[options.packages.find]
exclude =
Expand Down
16 changes: 16 additions & 0 deletions tests/fixtures.py
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@
import pathlib
import tempfile
import textwrap
import functools
import contextlib

from .py39compat import FS_NONASCII
Expand Down Expand Up @@ -294,3 +295,18 @@ def setUp(self):
# Add self.zip_name to the front of sys.path.
self.resources = contextlib.ExitStack()
self.addCleanup(self.resources.close)


def parameterize(*args_set):
"""Run test method with a series of parameters."""

def wrapper(func):
@functools.wraps(func)
def _inner(self):
for args in args_set:
with self.subTest(**args):
func(self, **args)

return _inner

return wrapper
8 changes: 8 additions & 0 deletions tests/test_main.py
View file
Open in desktop
Expand Up @@ -50,6 +50,14 @@ def test_new_style_classes(self):
self.assertIsInstance(Distribution, type)
self.assertIsInstance(MetadataPathFinder, type)

@fixtures.parameterize(
dict(name=None),
dict(name=''),
)
def test_invalid_inputs_to_from_name(self, name):
with self.assertRaises(Exception):
Distribution.from_name(name)


class ImportTests(fixtures.DistInfoPkg, unittest.TestCase):
def test_import_nonexistent_module(self):
Expand Down