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

Type annotate the public API #167

Merged
merged 21 commits into from
Nov 1, 2023
Merged

Type annotate the public API #167

merged 21 commits into from
Nov 1, 2023

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Apr 5, 2023

Closes #90

This makes it easier for tools to provide completion and enhances the
documentation of the API.
This makes it behave better with newer ruff versions, and removes
the unused isort configuration.
This enables the use of newer Python typing functionality without
compatibility shims.
This ensures that the documentation build uses a well-defined Python
version, rather than whatever happens to be the default for the
environment.
@pradyunsg pradyunsg marked this pull request as ready for review April 7, 2023 10:12
@pradyunsg pradyunsg requested a review from takluyver April 7, 2023 10:15
Copy link
Member

@layday layday left a comment

Choose a reason for hiding this comment

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

Should have py.typed marker file inside the pyproject_hooks package.

src/pyproject_hooks/_impl.py Outdated Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Outdated Show resolved Hide resolved
src/pyproject_hooks/_impl.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member

Thanks @pradyunsg - it looks like @layday has already done a detailed review of the annotations, so I'll skip doing that.

My inclination is to be fairly conservative with Python version support, since this is meant to be a boring piece of low-level infrastructure that doesn't change often. I see pip hasn't dropped 3.7 support yet, so I'd suggest we don't here either. But I'm not going to argue about this if you really want to.

Copy link
Member

@layday layday left a comment

Choose a reason for hiding this comment

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

I've type-checked this branch against the main branch of build; there was only one error, which looks like something that should be fixed on build's side:

src/build/__init__.py:180: error: Argument "runner" to "BuildBackendHookCaller" has incompatible type "Callable[[Sequence[str], Optional[str], Optional[Mapping[str, str]]], None]"; expected "Optional[SubprocessRunner]"  [arg-type]

(This is presumably because the protocol has default arguments.)

@layday
Copy link
Member

layday commented Apr 8, 2023

pypa/build#601

@pradyunsg
Copy link
Member Author

The latest commit makes it possible to do...

if TYPE_CHECKING:
    from pyproject_hooks import SubprocessRunner

And then annotate subprocess runners using that type.

@pradyunsg
Copy link
Member Author

Instead of leaving that as a comment, I've gone ahead and documented that.

@henryiii
Copy link
Contributor

henryiii commented Apr 8, 2023

Wouldn’t it make sense to allow SubprocessRunner at runtime on Python 3.8+? So this interface oddity will go away once 3.7 is dropped? Another idea (if it’s not runtime checkable) is to just make it a dummy object at runtime, so downstream code isn’t forced into extra TYPE_CHECKING checks.

@layday
Copy link
Member

layday commented Apr 8, 2023

That's what we do with the IsolatedEnv in build, it's a simple ABC at runtime on Python 3.7.

src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
src/pyproject_hooks/_impl.py Show resolved Hide resolved
@pradyunsg
Copy link
Member Author

On the basis of "this is better than status quo", I'm gonna go ahead and merge this.

There's a couple of potential follow ups to improve this further:

  • Using the newer typing syntax and more granular "semantic" types, that @gotmax23 has suggested.
  • Making the protocol available on Python 3.8+ at runtime (with an object on 3.7), as @henryiii suggested.

@pradyunsg pradyunsg merged commit 0b036b5 into pypa:main Nov 1, 2023
20 of 21 checks passed
@pradyunsg pradyunsg deleted the typing branch November 1, 2023 06:24
@pradyunsg
Copy link
Member Author

(I'll fix the documentation build separately)

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.

Add type hints to public API
5 participants