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

Inline some types #221

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Inline some types #221

merged 7 commits into from
Apr 4, 2024

Conversation

jaraco
Copy link
Owner

@jaraco jaraco commented Apr 4, 2024

Ref #215

  • Move type annotations into masks.py
  • ⚫ Fade to black.
  • 🧎‍♀️ Genuflect to the types.
  • Move 'overload' types inline.

from typing import Any, Callable


def compose(*funcs: Callable[..., Any]) -> Callable[..., Any]:
Copy link
Owner Author

Choose a reason for hiding this comment

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

I observe that this type signature has been updated and again upstream, so this signature is now stale. At the very least, we should move these vendored functions into their own modules so they're easier to keep separate from novel logic.

@jaraco jaraco force-pushed the feature/inline-types branch from a3035b4 to f559977 Compare April 4, 2024 09:26
@jaraco jaraco force-pushed the feature/inline-types branch 2 times, most recently from 63b5297 to 145a392 Compare April 4, 2024 09:36
@jaraco jaraco force-pushed the feature/inline-types branch from 145a392 to eed813b Compare April 4, 2024 09:37
@jaraco
Copy link
Owner Author

jaraco commented Apr 4, 2024

I need to get this change merged or abandon it because it's too sensitive to conflicts.

@jaraco
Copy link
Owner Author

jaraco commented Apr 4, 2024

Docs builds are failing with:

/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.chunks:1: WARNING: py:class reference target not found: OpenTextMode
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.chunks:1: WARNING: py:class reference target not found: OpenBinaryMode
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenTextMode
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: _io.TextIOWrapper
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenBinaryMode
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: _io.FileIO
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenBinaryModeUpdating
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: Literal[-1, 1]
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: _io.BufferedRandom
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenBinaryModeReading
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: Literal[-1, 1]
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: _io.BufferedReader
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenBinaryModeWriting
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: Literal[-1, 1]
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: _io.BufferedWriter
/Users/jaraco/code/jaraco/path/path/__init__.py:docstring of path.Path.open:1: WARNING: py:class reference target not found: OpenBinaryMode

@jaraco
Copy link
Owner Author

jaraco commented Apr 4, 2024

Best I can tell, those types aren't documented in Sphinx anywhere. The typeshed readme points to https://typing.readthedocs.io, but adding that to the intersphinx links doesn't help with the errors, so probably we just need to exempt those from nitpick.

jaraco added 2 commits April 4, 2024 05:49
Suppress nitpicky checks for missing typeshed definitions.
@jaraco jaraco changed the title Inline the types Inline some types Apr 4, 2024
@jaraco jaraco marked this pull request as ready for review April 4, 2024 09:59
@jaraco jaraco merged commit 73fa139 into main Apr 4, 2024
24 checks passed
@jaraco jaraco deleted the feature/inline-types branch April 4, 2024 09:59
Copy link
Contributor

@bswck bswck left a comment

Choose a reason for hiding this comment

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

@jaraco A small suggestion you may or may not want to apply (due to extra complexity). It can be useful to keep typing-related things away from "polluting" runtime.

Comment on lines +687 to +770
@overload
def open(
self,
mode: OpenTextMode = ...,
buffering: int = ...,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Optional[Callable[[str, int], int]] = ...,
) -> TextIOWrapper: ...

@overload
def open(
self,
mode: OpenBinaryMode,
buffering: Literal[0],
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> FileIO: ...

@overload
def open(
self,
mode: OpenBinaryModeUpdating,
buffering: Literal[-1, 1] = ...,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> BufferedRandom: ...

@overload
def open(
self,
mode: OpenBinaryModeReading,
buffering: Literal[-1, 1] = ...,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> BufferedReader: ...

@overload
def open(
self,
mode: OpenBinaryModeWriting,
buffering: Literal[-1, 1] = ...,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> BufferedWriter: ...

@overload
def open(
self,
mode: OpenBinaryMode,
buffering: int,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> BinaryIO: ...

@overload
def open(
self,
mode: str,
buffering: int = ...,
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
newline: Optional[str] = ...,
closefd: bool = ...,
opener: Callable[[str, int], int] = ...,
) -> IO[Any]: ...

Copy link
Contributor

@bswck bswck Apr 10, 2024

Choose a reason for hiding this comment

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

To reduce runtime overhead, you could move all the overloads into an if TYPE_CHECKING block too.
It will

  • not create temporary functions
  • let you move typing.overload import into type checking imports, keeping it away from the runtime variable namespace

while still working out well in type checking. Similarly for other overloads.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You know what would be even nicer is if the type system provided a way to indicate that a function or method is a wrapper around another function/method and takes the exact same or slightly altered arguments, something like:

diff --git a/path/__init__.py b/path/__init__.py
index c803459..d1441d9 100644
--- a/path/__init__.py
+++ b/path/__init__.py
@@ -689,90 +689,7 @@ class Path(str):
     #
     # --- Reading or writing an entire file at once.
 
-    @overload
-    def open(
-        self,
-        mode: OpenTextMode = ...,
-        buffering: int = ...,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] | None = ...,
-    ) -> TextIOWrapper: ...
-
-    @overload
-    def open(
-        self,
-        mode: OpenBinaryMode,
-        buffering: Literal[0],
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> FileIO: ...
-
-    @overload
-    def open(
-        self,
-        mode: OpenBinaryModeUpdating,
-        buffering: Literal[-1, 1] = ...,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> BufferedRandom: ...
-
-    @overload
-    def open(
-        self,
-        mode: OpenBinaryModeReading,
-        buffering: Literal[-1, 1] = ...,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> BufferedReader: ...
-
-    @overload
-    def open(
-        self,
-        mode: OpenBinaryModeWriting,
-        buffering: Literal[-1, 1] = ...,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> BufferedWriter: ...
-
-    @overload
-    def open(
-        self,
-        mode: OpenBinaryMode,
-        buffering: int,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> BinaryIO: ...
-
-    @overload
-    def open(
-        self,
-        mode: str,
-        buffering: int = ...,
-        encoding: str | None = ...,
-        errors: str | None = ...,
-        newline: str | None = ...,
-        closefd: bool = ...,
-        opener: Callable[[str, int], int] = ...,
-    ) -> IO[Any]: ...
-
+    @typing.reflect
     def open(self, *args, **kwargs):
         """Open this file and return a corresponding file object.

Then, a reader of this could wouldn't be accosted by the copy-pasta from the upstream implementation and the type system could readily reflect the overloaded types.

Or even better, make that the default behavior, so that if no type signature is applied, the signature is implied by the way it's used.

Copy link
Owner Author

Choose a reason for hiding this comment

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

To reduce runtime overhead, you could move all the overloads into a TYPE_CHECKING block too.

Proposed in #224. Is that what you meant?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is something like typing.reflect already, but it's not a well-documented feature :) Namely, functools.wraps.

Copy link
Contributor

@bswck bswck Apr 10, 2024

Choose a reason for hiding this comment

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

The reason I think it's a type-checking equivalent of our typing.reflect is that functools.wraps takes a callable of signature P and the wrapper is then typed to be of signature P too.

Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce runtime overhead, you could move all the overloads into a TYPE_CHECKING block too.

Proposed in #224. Is that what you meant?

Yeah, gonna reply there.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is something like typing.reflect already, but it's not a well-documented feature :) Namely, functools.wraps.

I'm familiar with functools.wraps, but I've not seen it used except for it's documented primary purpose (wrapping a function in a decorator). I've created #225 to explore that possibility.

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.

2 participants