Skip to content

Allow types-foo to depend on foo #159

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

Merged
merged 3 commits into from
Mar 4, 2025
Merged

Allow types-foo to depend on foo #159

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
915e5b7
Allow types-* to depend on *
Avasam Feb 14, 2025
f4a44c5
Merge branch 'main' of https://github.com/typeshed-internal/stub_uplo…
Avasam Mar 3, 2025
471b21f
Slight simplification
Avasam Mar 3, 2025
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
12 changes: 7 additions & 5 deletions stub_uploader/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@
import os
import re
import tarfile
import tempfile
import urllib.parse
from collections.abc import Generator, Iterable
from glob import glob
from pathlib import Path
import tempfile
from typing import Any, Optional

import requests
import tomli
from packaging.requirements import Requirement
from packaging.specifiers import Specifier, InvalidSpecifier
from packaging.specifiers import InvalidSpecifier, Specifier

from .const import META, THIRD_PARTY_NAMESPACE, TYPES_PREFIX, UPLOADED_PATH

Expand Down Expand Up @@ -350,9 +350,11 @@ def verify_external_req_stubs_require_its_runtime(

runtime_req_name = EXTERNAL_RUNTIME_REQ_MAP.get(req.name, req.name)

if not runtime_in_upstream_requires(
req, data
) and not runtime_in_upstream_sdist_requires(req, data):
if (
runtime_req_name != upstream_distribution # Allow `types-foo` to require `foo`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you want me to hide this check behind a function for consistency, I'd call it runtime_is_upstream.

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 this is the bug that I alluded to when I wrote the refactoring PR. But wouldn't it be clearer to just check req.name == upstream_distribution at the beginning of the function and exit early? This would also skip fetching the data from PyPI.

Copy link
Contributor Author

@Avasam Avasam Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But wouldn't it be clearer to just check req.name == upstream_distribution at the beginning of the function and exit early? This would also skip fetching the data from PyPI.

If we don't mind not validating that upstream_distribution does exist on pypi in this case.
Of course for any existing stubs on typeshed we'd assume that its upstream does exist on pypi.

But let's say someone adds a new stubs, mistyped the distribution's name, and makes it reference on itself. It's quite the edge case, but to be considered.

Hmm, I guess stubtest and maybe other tools as well would fail. So probably fine.

Copy link
Contributor Author

@Avasam Avasam Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the bug that I alluded to when I wrote the refactoring PR. But wouldn't it be clearer to just check req.name == upstream_distribution

I'm not sure which side the the EXTERNAL_RUNTIME_REQ_MAP makes sense here. (and tbh I may be too tired rn to make sense of it ^^") But I was reusing what's in runtime_in_upstream_sdist_requires and runtime_in_upstream_requires

Edit: I think it doesn't really matter because we wouldn't self reference, let's say django-stubs, and this mapping specifically lists stubs outside typeshed. So yeah, simpler to use req.name directly.

and not runtime_in_upstream_requires(req, data)
and not runtime_in_upstream_sdist_requires(req, data)
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative syntax:

Suggested change
if (
runtime_req_name != upstream_distribution # Allow `types-foo` to require `foo`
and not runtime_in_upstream_requires(req, data)
and not runtime_in_upstream_sdist_requires(req, data)
):
if not (
runtime_req_name == upstream_distribution # Allow `types-foo` to require `foo`
or runtime_in_upstream_requires(req, data)
or runtime_in_upstream_sdist_requires(req, data)
):

raise InvalidRequires(
f"Expected dependency {runtime_req_name} to be listed in {upstream_distribution}'s "
+ "requires_dist or the sdist's *.egg-info/requires.txt"
Expand Down
2 changes: 2 additions & 0 deletions tests/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ def test_verify_external_req() -> None:
verify_external_req(
Requirement("mypy-extensions"), "mypy", _unsafe_ignore_allowlist=True
)
# Check that types-foo can depend on foo
verify_external_req(Requirement("setuptools"), "setuptools")

with pytest.raises(
InvalidRequires, match="to be present in the stub_uploader allowlist"
Expand Down