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

feat: allow classmethods to receive Py<PyType> #3587

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 20, 2023

This is necessary for async classmethods

@adamreichold
Copy link
Member

Could you add a test case and change log entry for this? Thanks!

@davidhewitt
Copy link
Member

Also, if you're willing, it would be great to also support this for #[pyfunction(pass_module)] please. I think that will have the same problem.

@wyfo
Copy link
Contributor Author

wyfo commented Nov 20, 2023

Also, if you're willing, it would be great to also support this for #[pyfunction(pass_module)] please. I think that will have the same problem.

Indeed, I've added it. I had to modify type_is_pymodule function, but I am wondering if this function is really useful, as there is no check like this for #[classmethod].

Copy link

codspeed-hq bot commented Nov 20, 2023

CodSpeed Performance Report

Merging #3587 will improve performances by 22.03%

Comparing wyfo:classmethod_into (744de3a) with main (abe518d)

Summary

⚡ 2 improvements
✅ 76 untouched benchmarks

Benchmarks breakdown

Benchmark main wyfo:classmethod_into Change
list_via_downcast 153.9 ns 126.1 ns +22.03%
not_a_list_via_downcast 153.9 ns 126.1 ns +22.03%

@davidhewitt
Copy link
Member

👍

@wyfo similar finishing request here - can you please squash it? Also, if you're willing to check the patch diff on codecov and add an additional test or two to hit the coverage target, that would be massively appreciated 🙏

@wyfo
Copy link
Contributor Author

wyfo commented Nov 22, 2023

Actually, the issue with coverage comes from the error cases pyfunction::type_is_pymodule, because I've added 4 of them. But they cannot be covered, because test would not compile; actually, I thought that UI test would be counted in coverage, but it seems not, so I've no way to add coverage for these branches. Have I missed something here?
By the way, I've proposed to remove pyfunction::type_is_pymodule, because it's redundant with the type checking, and not consistent with #[classmethod] which doesn't have a macro-level check. It would solve the coverage issue.

This is necessary for async functions
@davidhewitt
Copy link
Member

Ah makes sense, let's merge this and I'll have a play with what the error message looks like with that check removed 👍

@davidhewitt davidhewitt added this pull request to the merge queue Nov 22, 2023
Merged via the queue into PyO3:main with commit 3f0dfa9 Nov 22, 2023
35 of 36 checks passed
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