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

Don't use intocallback in method macros #2664

Merged
merged 8 commits into from
Oct 16, 2022

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Oct 6, 2022

This makes the errors much nicer.

src/callback.rs Outdated Show resolved Hide resolved
src/pyclass.rs Show resolved Hide resolved
tests/ui/missing_intopy.stderr Show resolved Hide resolved
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! I guess this goes in "changed" section of changelog?

There's similar for the proto methods, do you think we can use a similar approach?

@mejrs
Copy link
Member Author

mejrs commented Oct 6, 2022

There's similar for the proto methods, do you think we can use a similar approach?

Can users still call any callback trait since pyproto was removed?

Also..ugh

  = help: the following other types implement trait `IntoPy<T>`:
            <&'a OsString as IntoPy<Py<PyAny>>>
            <&'a Path as IntoPy<Py<PyAny>>>
            <&'a PathBuf as IntoPy<Py<PyAny>>>
            <&'a PyErr as IntoPy<Py<PyAny>>>
            <&'a String as IntoPy<Py<PyAny>>>
            <&'a [u8] as IntoPy<Py<PyAny>>>
            <&'a str as IntoPy<Py<PyAny>>>
            <&'a str as IntoPy<Py<PyString>>>
          and 172 others

How do we deal with this in CI again? These counts change depending on the features 😭

I have filed rust-lang/rust#102752 which should do away both with the horrible error message and the "and ... others" thing, so these errors should get even better :)

@mejrs mejrs force-pushed the we_love_better_errors branch from 09ca410 to 391a65f Compare October 8, 2022 16:42
@mejrs
Copy link
Member Author

mejrs commented Oct 8, 2022

Okay, I think this works.

@mejrs
Copy link
Member Author

mejrs commented Oct 8, 2022

Ah, I have a better idea,

@mejrs mejrs requested a review from davidhewitt October 8, 2022 22:42
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, this is looking really good! Sorry for my extended delay on reply, my son and I have been ill over the weekend previous and I'm finally better and catching up with things.

Can users still call any callback trait since pyproto was removed?

So eventually I would like to remove src/callback.rs and put everything in it into impl_ details. Users shouldn't be using that stuff directly as far as I'm concerned.

For the __dunder__ methods, the generated code is built up in pieces (because it needed to be flexible to support the variety of forms those dunders take).

The last uses of callback::convert which might be doing ok-wrapping are here and here.

If you want, we could save those for a separate PR?

src/pyclass.rs Show resolved Hide resolved
<PyErr as From<&PyArithmeticError>>
<PyErr as From<&PyAssertionError>>
<PyErr as From<&PyAttributeError>>
and $N others
Copy link
Member

Choose a reason for hiding this comment

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

Aha nice find!

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a find, I made it 😉

@mejrs
Copy link
Member Author

mejrs commented Oct 15, 2022

Thanks, this is looking really good! Sorry for my extended delay on reply, my son and I have been ill over the weekend previous and I'm finally better and catching up with things.

Get well soon 👍

If you want, we could save those for a separate PR?

I think that's be a good idea

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.

Cool, in which case let's merge this 👍

Get well soon 👍

Thanks, I'm fine now but my son still getting over it.

@davidhewitt davidhewitt merged commit 4a04603 into PyO3:main Oct 16, 2022
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