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

util: add project_wheel_metadata #340

Merged
merged 2 commits into from
Sep 16, 2021
Merged

util: add project_wheel_metadata #340

merged 2 commits into from
Sep 16, 2021

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Aug 5, 2021

Signed-off-by: Filipe Laíns lains@riseup.net

@FFY00 FFY00 force-pushed the plumbing-module branch 2 times, most recently from 700d550 to 146ea2e Compare August 5, 2021 13:43
setup.cfg Outdated
@@ -86,6 +87,7 @@ extend-ignore = E203
[mypy]
ignore_missing_imports = True
strict = True
implicit_reexport = True
Copy link
Member

Choose a reason for hiding this comment

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

This is gonna break --strict downstream. Just add __all__ in build.util.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for SimplePath, which is not explicitly re-exported in importlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

If so you want to set that option for only that lib, not globally. Or add a type ignore at the location of the import.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need to import SimplePath after python/importlib_metadata#335 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move to pyproject.toml mypy config, it's much cleaner and makes per-lib settings prettier if we need them. Not in this PR, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, feel free to open a PR.

from importlib_metadata import PackageMetadata, SimplePath


def _project_wheel_metadata(builder: build.ProjectBuilder) -> 'PackageMetadata':
Copy link
Member

Choose a reason for hiding this comment

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

Does it return PackageMetadata if you don't have importlib-metadata >= 4.1.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also on python3.8 or later this packagemetadata import would fail not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Older versions are not typed, but they do return an object that implements the protocol, tox -e py36-min works fine, and it pins importlib-metadata==0.22. I can bump the version though, if you think it would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally assume that static typing only supports a narrow set of versions (usually exactly pinned versions to the latest dependencies). I wouldn't worry about bumping a runtime version for typing, or about whether older versions fail typing checks. So I'm happy with it as is.

Copy link
Member

Choose a reason for hiding this comment

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

but they do return an object that implements the protocol

The protocol declares json, which does not exist on email.Message, and only a small subset of email.Message's methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, right. We need to keep the type hints and the implementation in sync then. I think the best option is for us to declare the fixed SimplePath protocol and cast it, or simply have a # type: ignore instead of pulling importlib_metadata for Python >= 3.8 only when type checking.

Copy link
Member

Choose a reason for hiding this comment

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

Or just return the distribution and let consumers call .metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

The distribution is dependent on the metadata path being on the system. We would have to make this a context manager to do the cleanup of the temporary directory, which is just more boilerplate for people to write.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, we would need to cast to the incorrect SimplePath, which will break in the future. I just went with a # type: ignore.

src/build/util.py Outdated Show resolved Hide resolved
@layday
Copy link
Member

layday commented Aug 17, 2021

There's a new version of importlib-metadata out if you have time to revisit this PR @FFY00.

@FFY00 FFY00 force-pushed the plumbing-module branch 2 times, most recently from 43a66e1 to d1bec97 Compare August 26, 2021 01:22
import build.env


if sys.version_info >= (3, 8) and not typing.TYPE_CHECKING:
Copy link
Member

Choose a reason for hiding this comment

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

and not typing.TYPE_CHECKING?

Copy link
Member Author

Choose a reason for hiding this comment

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

To make it use the fixed SimplePath protocol in importlib-metadata.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like that we're mixing runtime with type checking checks. Was the broken protocol shipped as part of the stdlib? Otherwise this shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, https://github.com/python/cpython/blob/8868d48712aee2b490efaf60bed8dfe9fb14d6b7/Lib/importlib/metadata/_meta.py#L32.

Do you have any alternative? We know the code in importlib.metadata is compatible, it's just the protocol is wrong. I don't see another way of making mypy happy without adding a dependency on importlib_metadata for Python < 3.11, without it being actually needed at runtime, or sprinkling some # type: ignore comments.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just pull TYPE_CHECKING out of the version check:

diff --git a/src/build/util.py b/src/build/util.py
index cd2b95a..f56901a 100644
--- a/src/build/util.py
+++ b/src/build/util.py
@@ -13,18 +13,15 @@
 import build
 import build.env

-
-if sys.version_info >= (3, 8) and not typing.TYPE_CHECKING:
+if typing.TYPE_CHECKING:
+    import importlib_metadata
+elif sys.version_info >= (3, 8):
     import importlib.metadata as importlib_metadata
 else:
     import importlib_metadata


-if typing.TYPE_CHECKING:
-    from importlib_metadata import PackageMetadata
-
-
-def _project_wheel_metadata(builder: build.ProjectBuilder) -> 'PackageMetadata':
+def _project_wheel_metadata(builder: build.ProjectBuilder) -> 'importlib_metadata.PackageMetadata':
     with tempfile.TemporaryDirectory() as tmpdir:
         path = pathlib.Path(builder.metadata_path(tmpdir))
         # https://github.com/python/importlib_metadata/pull/343
@@ -34,7 +31,7 @@ def _project_wheel_metadata(builder: build.ProjectBuilder) -> 'PackageMetadata':
 def project_wheel_metadata(
     srcdir: Union[str, 'os.PathLike[str]'],
     isolated: bool = True,
-) -> 'PackageMetadata':
+) -> 'importlib_metadata.PackageMetadata':
     """
     Return the wheel metadata for a project.

Copy link
Member Author

Choose a reason for hiding this comment

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

This protocol is still not fixed in importlib_metadata (needs python/importlib_metadata#342, see python/importlib_metadata#343), so I think we should just go ahead with the # type: ignore as we know the code is definitely correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should get a message about an unnecessary type ignore when this does get fixed, so don't see any harm.

return importlib_metadata.PathDistribution(path).metadata # type: ignore


def project_wheel_metadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an __all__ with this in it to mark this method as public 👍 (and hides importlib_metadata)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also usually recommend:

def __dir__() -> Tuple[Any, ...]:
    return __all__

This fixes tab completion the REPL or IPython in Python 3.7+ to only show public methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am gonna do __all__ to keep in sync with the other modules, and then you can open a PR moving to that form.

tests/packages/test-metadata/pyproject.toml Show resolved Hide resolved
Signed-off-by: Filipe Laíns <lains@riseup.net>
…ibutions

We are now pulling importlib_metadata in the mypy run, which makes mypy
raise these errors, as importlib_metadata is only partially typed.

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Sep 16, 2021

Does anyone have other comments?

@layday layday self-requested a review September 16, 2021 20:54
@layday
Copy link
Member

layday commented Sep 16, 2021

LGTM!

@FFY00 FFY00 merged commit ea50e7c into pypa:main Sep 16, 2021
@FFY00 FFY00 deleted the plumbing-module branch September 16, 2021 21:13
inmantaci pushed a commit to inmanta/inmanta-core that referenced this pull request Sep 17, 2021
Bumps [build](https://github.com/pypa/build) from 0.6.0.post1 to 0.7.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/pypa/build/blob/main/CHANGELOG.rst">build's changelog</a>.</em></p>
<blockquote>
<h1>0.7.0 (16-09-2021)</h1>
<ul>
<li>Add <code>build.util</code> module with an high-level utility API (<code>PR [#340](https://github.com/pypa/build/issues/340)</code>_)</li>
</ul>
<p>.. _PR <a href="https://github-redirect.dependabot.com/pypa/build/issues/340">#340</a>: <a href="https://github-redirect.dependabot.com/pypa/build/pull/340">pypa/build#340</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/pypa/build/commit/c940180b6ace5210e6f00149d45b1358f39fb274"><code>c940180</code></a> release 0.7.0</li>
<li><a href="https://github.com/pypa/build/commit/ea50e7cd2c655eb2837f010cbd2d115fd199da43"><code>ea50e7c</code></a> build/env: ignore no-untyped-call errors on importlib.metadata.distyributions</li>
<li><a href="https://github.com/pypa/build/commit/a9a7d791b1be217827458e6a464ae7a42111556f"><code>a9a7d79</code></a> util: add project_wheel_metadata</li>
<li><a href="https://github.com/pypa/build/commit/e774f5e04f1e529d3405e94fec957547b6a6145c"><code>e774f5e</code></a> pre-commit: bump repositories</li>
<li><a href="https://github.com/pypa/build/commit/e4b9b05ad91ce01e26a41f477739c5e8a8ae6bd8"><code>e4b9b05</code></a> tox: show error codes in mypy</li>
<li><a href="https://github.com/pypa/build/commit/450097a47050a1d8da6cb2b151dde8ccc902690a"><code>450097a</code></a> tests: integration: use PEP 440 version as setuptools-scm fallback</li>
<li><a href="https://github.com/pypa/build/commit/5aeef1bde39e07851e970b3f1876c1bc191ef833"><code>5aeef1b</code></a> pre-commit: bump repositories</li>
<li><a href="https://github.com/pypa/build/commit/f20755c59995675a15e798023301b8459d6ed2c6"><code>f20755c</code></a> pre-commit: setup ci (<a href="https://github-redirect.dependabot.com/pypa/build/issues/344">#344</a>)</li>
<li><a href="https://github.com/pypa/build/commit/6d73774c6f2cf633908a475f4b82405af0320056"><code>6d73774</code></a> main: fix missing dependencies error</li>
<li>See full diff in <a href="https://github.com/pypa/build/compare/0.6.0.post1...0.7.0">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=build&package-manager=pip&previous-version=0.6.0.post1&new-version=0.7.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants