Skip to content

fileinput: Fix TypeVar usage #7934

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

Merged
merged 10 commits into from
May 24, 2022
Merged

Conversation

JelleZijlstra
Copy link
Member

Fixes #7922, part of #7928.

Also discovered an apparent CPython bug in the process: python/cpython#93157

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

The docs say:

With mode you can specify which file mode will be passed to open(). It must be one of 'r', 'rU', 'U' and 'rb'.

https://docs.python.org/3/library/fileinput.html#fileinput.FileInput

But 'rU' and 'U' have been deprecated since Python 3.4, so perhaps it doesn't matter

@JelleZijlstra
Copy link
Member Author

JelleZijlstra commented May 24, 2022

Good catch! U support was removed in 3.11 (python/cpython@19ba212) and I didn't check the earlier branches carefully enough. I'll add it.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

else:
_TextMode: TypeAlias = Literal["r", "rU", "U"]

_AnyStr_co = TypeVar("_AnyStr_co", str, bytes, covariant=True)
Copy link
Member

Choose a reason for hiding this comment

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

We define the same TypeVar (with the same name) in builtins.pyi and os/__init__.pyi — is it worth adding it to _typeshed, do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's simple enough that I don't mind redefining it, but I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

We can leave it for now

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Comment on lines +247 to +248
# Because mode isn't keyword-only here yet, we need two overloads each for
# the bytes case and the fallback case.
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 annoying, but it doesn't look like there's any other way of doing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sebastian opened a conversation a while ago about allowing arguments without defaults to follow arguments with defaults, which would make this easier, but it didn't go anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There was some pushback, because that's mostly useful for typing and considered bad API design otherwise, but honestly more from typing-critical developers, if I remember correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Plus ça change, plus c'est la même chose

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative could be to add a special marker like typing.NO_DEFAULT, which we'd use as the default. That would look like this:

        @overload
        def __init__(
            self: FileInput[bytes],
            files: StrOrBytesPath | Iterable[StrOrBytesPath] | None = ...,
            inplace: bool = ...,
            backup: str = ...,
            bufsize: int = ...,
            mode: Literal["rb"] = NO_DEFAULT,
            openhook: Callable[[StrOrBytesPath, str], _HasReadlineAndFileno[bytes]] | None = ...,
        ) -> None: ...

And the semantics would be that type checkers don't pick the overload if no value is given for mode, but it's still the 6th positional argument.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative could be to add a special marker like typing.NO_DEFAULT, which we'd use as the default.

Oh, I like that quite a lot, actually. It would mean we wouldn't have to change Python's grammar at all. It does also feel like the kind of thing that would disproportionately benefit typeshed, 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.

There was also a recent mypy issue where end-users were complaining about this, so it might be more broadly useful. If @srittau likes it too I can send it to typing-sig.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be possible to write a script that automatically applies NO_DEFAULT in typeshed wherever possible. I would be interested to see the results,

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was the thread. Looking back at it, the push back wasn't too bad. Maybe it would be worth trying again with a complete PEP?

I'm not particularly fond of typing.NO_DEFAULT, but would prefer it over the status quo.

Copy link
Member

@AlexWaygood AlexWaygood May 24, 2022

Choose a reason for hiding this comment

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

I think I actually prefer typing.NO_DEFAULT. The status quo is definitely annoying for type annotations, but I think syntax changes should have to meet a pretty high bar, and I'm not sure this meets that bar ://

typing.NO_DEDAULT feels like it would be much simpler to implement with minimal changes to the language itself (and it would also be much easier to backport).

@AlexWaygood AlexWaygood merged commit d950ec3 into python:master May 24, 2022
@JelleZijlstra JelleZijlstra deleted the fileinput branch May 24, 2022 16:57
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.

Wrong stub for fileinput.input()
4 participants