Skip to content

Workaround to stubtest to handle pyOpenSSL correctly #10663

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

Closed
wants to merge 7 commits into from

Conversation

jolaf
Copy link
Contributor

@jolaf jolaf commented Jun 17, 2021

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
Copy link
Contributor

Akuli commented Jun 17, 2021

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
Member

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

@jolaf
Copy link
Contributor Author

jolaf commented Jun 17, 2021

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
Copy link
Contributor

Akuli commented Jun 17, 2021

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
Member

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

@Akuli
Copy link
Contributor

Akuli commented Jun 17, 2021

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

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

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
Copy link
Contributor Author

jolaf commented Jun 17, 2021

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