feat: add #[pyo3(from_item_all)] for FromPyObject

[BlueGlassBlock]

This PR addresses #3112 by implementing #[pyo3(item_all)] #[pyo3(from_item_all)].

[adamreichold]

I think UI tests exercising the error paths would be good to have here.

[BlueGlassBlock]

I think UI tests exercising the error paths would be good to have here.

I've noticed that, and I'll figure out how to fix the tests and improve coverage today (that's why I marked this as a draft⁠) (⁠^⁠^⁠)

[davidhewitt]

It looks like you may have run the UI tests with nightly Rust? Quite a few spans have changed. Suggest rebuilding with stable.

Overall I'm happy for this functionality although I'm unsure about the name item_all. I understand why you chose it though it's not immediately intuitive to me as a reader what it means.

I wonder if something like all_from_items or from_item_all are any clearer? I don't love those either so happy for further suggestions!

[BlueGlassBlock]

I personally perfer from_item_all, since that's similar to get_all and set_all which we already have.

I'll update the PR again once I can continue to work on it.

[BlueGlassBlock]

It seemed that I forgot some paths in the UI tests. I'll address them soon.

I'm uncertain about the name too. from_items or something like that seems okay to me.

I'm thinking about the divergence in behavior from get_all and set_all, which don't allow specifying the name with get, name = ..., so that seems inconsistent. However, I think allowing specifying args passed to get_item is still useful. Maybe I could separate that to an extra proposal along with changes to get_all and set_all?

[davidhewitt]

I'm thinking about the divergence in behavior from get_all and set_all, which don't allow specifying the name with get, name = ..., so that seems inconsistent.

This is a good point and something we overlooked when originally implementing get_all and set_all, it would be nice to allow them to accept #[pyo3(name = ...)] on individual fields.

[BlueGlassBlock]

Ugh. I didn't manage to make coverage rate pass codecov's check. Are declarative macro invocations blocking llvm-cov to pick them up?

[davidhewitt]

This looks fine to me, the implementation is comprehensive. For some reason the coverage isn't reporting the error cases from the UI tests, but I see them here, and that normally works, so I think we are good to merge this. Thanks!

bors r+

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion