-
Notifications
You must be signed in to change notification settings - Fork 783
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
clean up lib.rs #1868
clean up lib.rs #1868
Conversation
mejrs
commented
Sep 13, 2021
- moves serde.rs to /conversions, like the other optional features
- moves the proc macro imports to the top level so they are more discoverable on the front page
- moves the link declarations in lib.rs down, to remove the noise of defining links inside the text itself
- moves the macro_rules! macros to macros.rs to declutter lib.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a much-appreciated tidy-up! One thought on the proc macro imports...
src/lib.rs
Outdated
$crate::py_run_impl!($py, *$dict, &$crate::unindent::unindent($code)) | ||
}}; | ||
} | ||
pub use pyo3_macros::{pyclass, pyfunction, pymethods, pymodule, pyproto}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add a FromPyObject
import here so that users can write #[derive(pyo3::FromPyObject)]
? (I think we can also switch to using this in test_proc_macro_hygiene
.)
And if we add that, I think it's equivalent to
pub use pyo3_macros::{pyclass, pyfunction, pymethods, pymodule, pyproto}; | |
pub use pyo3_macros::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've imported it explicitly. If I do that with a glob import, this happens, which makes it really confusing where a macro is coming from.
pub use pyo3_macros::*;
#[macro_use]
mod macros;
@@ -7,3 +7,4 @@ pub mod num_bigint; | |||
pub mod num_complex; | |||
mod osstr; | |||
mod path; | |||
pub mod serde; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 it doesn't feel exactly like a conversion to me but I do like having it in the same place as all the other optional dependencies!
Looks like CI is breaking on windows runners, see also rust-lang/rust#88924 |
Mmm, following that thread it looks like Github did something which has now been reverted! 😬 |