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

Export warning classes and add PyErr::warn_explicit() #2742

Merged
merged 1 commit into from
Nov 17, 2022
Merged

Conversation

birkenfeld
Copy link
Member

Fixes #2741

Copy link
Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM, two comments:

  • would it be clearer if we moved warnings to pyo3::warning?
  • How hard would it be to add a method so we could call PyErr::warn_from_type::<PyUserWarning>(....) (like you can do dict.is_instance_of::<PyAny>)?

@birkenfeld
Copy link
Member Author

  1. there wouldn't be much in it, and since Warnings are Exceptions anyway...
  2. possible, but I feel the combinatoric explosion of methods (what about a counterpart for warn_explicit...) is not worth the few times when it is needed.

Of course, these are my opinions, other maintainers may disagree.

@birkenfeld birkenfeld force-pushed the warnings branch 2 times, most recently from 509dfd5 to 0aae986 Compare November 16, 2022 19:11
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 for this! I'm ok with pyo3::exceptions as a reasonable home for these for now, though I'm also not sure either way. It should be possible to re-home them in the future if we want.

As for the typed methods, I'm unconvinced we need them for now - users can always define wrappers e.g. user_warning(py, msg) to do exactly what you want.

unsafe {
error_on_minusone(
py,
ffi::PyErr_WarnExplicit(
Copy link
Member

Choose a reason for hiding this comment

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

It looks like PyPyErr_WarnExplicit does exist as a symbol in PyPy shared lib, so I think we can update the ffi definition and enable warn_explicit on pypy.

@@ -477,7 +477,26 @@ impl PyErr {
}

/// Issues a warning message.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps worth expanding this doc to say it's equivalent to warnings.warn (and similar for warn_explicit)?

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!

@birkenfeld birkenfeld merged commit 1d20f2a into main Nov 17, 2022
@birkenfeld birkenfeld deleted the warnings branch November 17, 2022 18:30
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.

export PyUserWarning
3 participants