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

Update create_exception macro to support submodules #2121

Closed
wants to merge 2 commits into from
Closed

Update create_exception macro to support submodules #2121

wants to merge 2 commits into from

Conversation

althonos
Copy link
Member

@althonos althonos commented Jan 23, 2022

Hi !

This PR updates the create_exception macro to support declaring the exception in a submodule. Currently, it is only possible to declare it in a top-level module.

Please consider adding the following to your pull request:

  • an entry in CHANGELOG.md
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

@davidhewitt
Copy link
Member

Hey, thanks for this!

I have a couple of thoughts / questions:

  1. I was hoping to eventually deprecate create_exception! and instead just recommend #[pyclass] to create custom exception types. See Add a proc macro for exceptions #295. Would you be willing to comment on whether that would suit your use-case / do you have any design opinions on the last comment on that thread?
  2. The rest of pyo3 has moved from #[pyclass(module = foo)] to #[pyclass(module = "foo.bar")] (i.e. module path as a literal instead of quotes). Should we perhaps be doing that here too, i.e. adding a new form for the macro which matches a literal instead of an ident? We could keep the existing form for backwards compatibility.

@althonos
Copy link
Member Author

I was hoping to eventually deprecate create_exception! and instead just recommend #[pyclass] to create custom exception types. See Add a proc macro for exceptions #295. Would you be willing to comment on whether that would suit your use-case / do you have any design opinions on the last comment on that thread?

Great! I'll have a look at the new way then.

The rest of pyo3 has moved from #[pyclass(module = foo)] to #[pyclass(module = "foo.bar")] (i.e. module path as a literal instead of quotes). Should we perhaps be doing that here too, i.e. adding a new form for the macro which matches a literal instead of an ident? We could keep the existing form for backwards compatibility.

I also realized that and was hoping to get the macro to accept both a literal (allowing fully qualified module names) or an identifier (for backward compatibility) but it resulted in more complicated macros (because you can't just casually stringify anymore) so i went for the simpler way and waited for feedback 😉

@davidhewitt
Copy link
Member

Great! Let me know what you think of the #[pyclass] way.

For modifying this macro to take literals - I was thinking it would be possible to make the "main" implementation match a literal, and then the ident form would just forward with stringify. Should be straightforward I think; but again I'd prefer we got the #[pyclass] functionality complete, killing the need for this macro at all.

@althonos
Copy link
Member Author

althonos commented Jan 24, 2022

For modifying this macro to take literals - I was thinking it would be possible to make the "main" implementation match a literal, and then the ident form would just forward with stringify.

That's what I thought, but unfortunately you cannot do:

macro_rules! create_exception {
    ($module: ident, $name: ident, $base: ty) => {
        create_exception!(stringify!($module), $name, $base);
    }
    ($module: literal, $name: ident, $base: ty, $doc: expr) => {
        ...
    }
}

because create_exception! will be evaluated before stringify!.

@davidhewitt
Copy link
Member

Ah, of course 😆

This pull request was closed.
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