Skip to content

Conversation

@KotlinIsland
Copy link
Contributor

@KotlinIsland KotlinIsland commented Jul 3, 2022

And it shouldn't be a sys._version_info, it should be a 5-tuple.

This is a 5-tuple, of the same form as sys.version_info, or, if the feature was dropped, or it's release date is undetermined, is None.

This is also needed for python/cpython#93628

And it shouldn't use `sys._version_info`
@github-actions

This comment has been minimized.


_version_info: TypeAlias = tuple[int, int, int, str, int]

class _Feature:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be cool to have this as a

_T_minver = TypeVar("_T_minver", bound=tuple[int, int, int, str, int])
_T_maxver = TypeVar("_T_maxver", bound=tuple[int, int, int, str, int] | None)

class _Feature(Generic[_T_minver, _T_maxver]):
    ...

So that the runtime constants can be exposed at type checking time

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 think that's necessary here tbh, and making classes generic in the stub when they're not at runtime can have some unfortunate consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexWaygood it doesn't narrow to the the literal values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or not...

Copy link
Member

Choose a reason for hiding this comment

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

_Feature is private so it's not exposed right?

It is documented, though. And I still think in general we shouldn't make things generic in the stub when they're not at runtime unless there's a compelling use case for it, which I don't really see here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you do something like:

class _Feature:
    ...

class _StubOnlyFeature(_Feature, Generic[...]):
    ...

Copy link
Member

Choose a reason for hiding this comment

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

Could you do something like:

class _Feature:
    ...

class _StubOnlyFeature(_Feature, Generic[...]):
    ...

I'm even less a fan of that ;)

I prefer making classes generic in the stub to introducing lies about what class certain objects are at runtime.

@AlexWaygood AlexWaygood merged commit 60a3cee into python:master Jul 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2022

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

@KotlinIsland
Copy link
Contributor Author

Now have to make sure mypy works with it...

@KotlinIsland
Copy link
Contributor Author

Mypy should be fine, it doesn't look at the values.

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.

3 participants