Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add _ssl module #11155

Merged
merged 24 commits into from
Oct 2, 2024
Merged

add _ssl module #11155

merged 24 commits into from
Oct 2, 2024

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Dec 12, 2023

Really all I needed for fixing the inheritance was _ssl._SSLContext. But then I needed all the other stuff in _ssl, and if I was doing that I wanted to do a thorough job of it.

Motivation was originally related to #3968 , but we're well beyond that now, really.

Really all I needed for fixing the inheritance was _ssl._SSLContext.
But then I needed all the other stuff in _ssl, and if I was doing that
I wanted to do a thorough job of it.

Motivation was originally related to python#3968 ,
but we're well beyond that now, really.
@tungol tungol marked this pull request as draft December 12, 2023 20:56

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@tungol tungol marked this pull request as ready for review December 12, 2023 22:19

This comment has been minimized.

stdlib/_ssl.pyi Outdated

class _EmptyCertInfo(TypedDict): ...

def _test_decode_cert(__path: str) -> _CertInfo: ...
Copy link
Member

Choose a reason for hiding this comment

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

In general, we prefer not to add private functions/attributes/classes unless somebody specifically needs them for something. This helps reduce the maintenance burden for us (since these are usually implementation details that often change in patch releases), and means that IDEs are less likely to have their autocomplete suggestions cluttered with these usually-undocumented internals. If a private class is used as a base class for a public class, then we've obviously got no choice except to include the private class in the stub -- but it looks like there's a few instances in this PR (like this function here) where you're adding private things that maybe aren't really necessary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense; I'll do some cleaning up.

@AlexWaygood
Copy link
Member

Motivation was originally related to #3968

Thanks so much for this series of PRs, by the way, I'm a big fan! Great to see all this stuff being cleaned up.

This comment has been minimized.

but add a comment about where _ssl.Certificate objects come from,
because that's just weird otherwise

This comment has been minimized.

def __lt__(self, other: object) -> bool: ...
def __ne__(self, other: object) -> bool: ...

# _ssl.Certificate is weird: it can't be instantiated or subclassed.
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 I removed _ssl._SSLObject which underlies ssl.SSLObject but isn't a formal base class for it. Doing so leaves _ssl.Certificate a little stranded.

It don't have an underscore prefix, so stubcheck complains if it's absent, but the only possible way to create an instance is via a method of _ssl._SSLSocket. Part of the reason I fleshed out the stubs for _SSLSocket in the first place was that I was trying to figure how to generate an instance of Certificate in order to observe its behavior.

I waffled on leaving the _SSLSocket stubs in, but I figured this comment would have the same effect without (as much) maintenance burden. On the other hand, that's probably outside the goals of typeshed, so I understand if it'd be better to remove the comment and just let Certificate be an oddity with no clear explanation.

because they're inherited from object

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stdlib/_ssl.pyi Outdated
Comment on lines 121 to 125
has_ticket: bool
id: bytes
ticket_lifetime_hint: int
time: int
timeout: int
Copy link
Member

Choose a reason for hiding this comment

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

These were all @properties in the previous version, which seems more accurate because they have no setters.

Suggested change
has_ticket: bool
id: bytes
ticket_lifetime_hint: int
time: int
timeout: int
@property
def has_ticket(self) -> bool: ...
@property
def id(self) -> bytes: ...
@property
def ticket_lifetime_hint(self) -> int: ...
@property
def time(self) -> int: ...
@property
def timeout(self) -> int: ...

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Oct 2, 2024

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

@JelleZijlstra JelleZijlstra merged commit 4f37d8f into python:main Oct 2, 2024
68 checks passed
@tungol tungol deleted the ssl branch October 2, 2024 21:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants