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

Rename ssl to tls #166

Closed
wants to merge 1 commit into from
Closed

Rename ssl to tls #166

wants to merge 1 commit into from

Conversation

pquentin
Copy link

@pquentin pquentin commented Dec 5, 2023

The ssl name clashes with the ssl module name and I intend to reuse this variable to carry information about the ASGI TLS extension.

The ssl name clashes with the ssl module name and I intend to reuse this
variable to carry information about the ASGI TLS extension.
@pgjones
Copy link
Owner

pgjones commented Dec 27, 2023

Given the extension is called tls, wouldn't the other way around make more sense? I'm also unsure as "ssl" is used in asyncio so seems the correct name.

@pquentin
Copy link
Author

pquentin commented Jan 1, 2024

Thanks for the review.

My initial idea was to avoid adding two keyword arguments, ssl and tls. The two next commits in that series are urllib3@ed84446 (turning tls in an optional dictionary) and urllib3@3dd8137 (actually implementing the extension - this one is still incomplete).

Do I understand correctly that you'd prefer having both an ssl boolean and a tls optional dictionary, with tls set only when ssl is true and the user has asked for the TLS extension?

@pgjones
Copy link
Owner

pgjones commented May 25, 2024

I'm not sure Hypercorn should always extract the TLS information - is this a costly operation?

@pquentin
Copy link
Author

Yes, it is costly (I can measure it if you would like) and should be behind an option.

@pgjones
Copy link
Owner

pgjones commented May 27, 2024

Makes sense, I'd be happy with the ssl bool and the tls dictionary if the latter is enabled.

@pquentin pquentin closed this May 27, 2024
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.

None yet

2 participants