Skip to content

Workaround to stubtest to handle pyOpenSSL correctly - #10663

Closed
jolaf wants to merge 7 commits into
python:masterfrom
jolaf:stubtest-openssl
Closed

jolaf wants to merge 7 commits into
python:masterfrom
jolaf:stubtest-openssl

Conversation

@jolaf

@jolaf jolaf commented Jun 17, 2021 •

Copy link
Copy Markdown
Contributor

Description

Currently, stubtest fails on pyOpenSSL stubs because pyOpenSSL has its submodules wrapped into cryptography.utils._ModuleWithDeprecations instances and some of the methods wrapped into cryptography.utils._DeprecatedValue instances.

See python/typeshed#5657 (comment) for details.

This PR adds additional checks to runtime field that allows unwrapping _ModuleWithDeprecations and _DeprecatedValue objects and extracting module and function objects from them.

Test Plan

After this PR is applied, tests for python/typeshed#5657 should pass without errors.

@Akuli

Akuli commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

This seems quite fragile. Personally I would just ignore it in typeshed. In general, stubtest isn't meant to work perfectly in every possible situation, which is why the exclude lists exist.

@JelleZijlstra

Copy link
Copy Markdown
Member

I agree with @Akuli, I don't think we should add this kind of module-specific logic.

@jolaf

jolaf commented Jun 17, 2021 •

Copy link
Copy Markdown
Contributor Author

This seems quite fragile. Personally I would just ignore it in typeshed. In general, stubtest isn't meant to work perfectly in every possible situation, which is why the exclude lists exist.

Well, I understand that, but after I've made this fix, stubtest found 4 more errors in my OpenSSL.crypto stubs, which seems useful and would not happen if stubtest was left unfixed.

@Akuli

Akuli commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

This will become unnecessary in the future when module-level __getattr__ (in .py files) becomes a thing. It can be used to implemented deprecated module members, and there's no longer any need for libraries to wrap the module in a custom class.

@JelleZijlstra

Copy link
Copy Markdown
Member

That future is here as of Python 3.7, for what it's worth.

@Akuli

Akuli commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

But pyopenssl still supports Python 2 (eww) and 3.6. https://github.com/pyca/pyopenssl/blob/main/setup.py

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, the improvements to pyOpenSSL have been great!

Like others here, I'm wary of special casing for specific packages in stubtest. I'd be potentially okay with it if it were a common idiom, but I don't think that's the case. value is a very common attribute name and this could cause some confusing things to happen.

@jolaf

jolaf commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

value is a very common attribute name and this could cause some confusing things to happen.

I see this would not be merged, but just in case, I've provided a more explicit check excluding any vagueness.

jolaf pushed a commit to jolaf/typeshed that referenced this pull request Jun 17, 2021
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.

4 participants