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

use extract_argument for #[setter] extraction #3998

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Mar 27, 2024

Following #3995 (comment)

This updates the #[setter] argument to extract_argument instead of FromPyObject::extract_bound. This allows extracting the same types as normal #[pyfunction] arguments, in particular &Bound. This also adds the deprecation warnings for gil-ref extraction.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple of suggestions for this one...

newsfragments/3998.fixed.md Show resolved Hide resolved
newsfragments/3998.fixed.md Outdated Show resolved Hide resolved
}
};

let holder = holders.push_holder(span);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO to rework this to use impl_arg_param later? I think the hardest part to doing so would be that impl_arg_param assumes the arguments are in an array and takes the array ident plus an index. We could probably rework that so that impl_arg_param just gets a TokenStream of something to extract from.

(Of course, if you're up for it, can also go for the refactoring now 👀)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added, the TODO for now, I think if we refactor this, this should also get rid of the from_py_with case, since that is handled in impl_arg_param already. I think it makes sense to do that, for consistency and code deduplication, but probably deserves it's own PR. (We would also need to figure out how to handle the Descriptor case)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, it would be a fair bit of rewiring, hopefully with a rewarding payoff at the end! 👍

@Icxolu Icxolu force-pushed the fix-setter-extraction branch from 690c5e0 to 1243232 Compare March 27, 2024 15:18
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! 👍

@davidhewitt davidhewitt enabled auto-merge March 27, 2024 15:35
@davidhewitt davidhewitt added this pull request to the merge queue Mar 27, 2024
Merged via the queue into PyO3:main with commit dd17102 Mar 27, 2024
42 of 43 checks passed
@Icxolu Icxolu deleted the fix-setter-extraction branch March 27, 2024 22:29
@Icxolu Icxolu mentioned this pull request Apr 4, 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.

2 participants