-
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
Expose py_run! macro #512
Expose py_run! macro #512
Conversation
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
==========================================
- Coverage 87.8% 87.54% -0.26%
==========================================
Files 65 65
Lines 3435 3428 -7
==========================================
- Hits 3016 3001 -15
- Misses 419 427 +8
Continue to review full report at Codecov.
|
src/lib.rs
Outdated
@@ -210,6 +213,96 @@ macro_rules! wrap_pymodule { | |||
}}; | |||
} | |||
|
|||
/// A convinient macro to execute a Python code snippet, with some local variables set. |
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.
typo: convenient
src/lib.rs
Outdated
/// format!("{}時{}分{}秒", self.hour, self.minute, self.second) | ||
/// } | ||
/// #[getter] | ||
/// fn hour(&self) -> PyResult<u32> { |
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.
(PyResult isn't necessary here)
src/lib.rs
Outdated
/// | ||
/// # Example | ||
/// ``` | ||
/// use pyo3::{prelude::*, PyRawObject, py_run}; |
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.
why is PyRawObject imported?
$py.run("import sys; sys.stderr.flush()", None, None) | ||
.unwrap(); | ||
}) | ||
.expect($code) |
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.
To be generally useful, I think this should not panic on exceptions, or reflect this in the name?
src/lib.rs
Outdated
#[doc(hidden)] | ||
pub fn _indoc_runtime(commands: &str) -> String { | ||
let indent; | ||
if let Some(second) = commands.lines().nth(1) { |
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.
better: let indent = if ...
src/lib.rs
Outdated
pub fn _indoc_runtime(commands: &str) -> String { | ||
let indent; | ||
if let Some(second) = commands.lines().nth(1) { | ||
indent = second |
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.
Some('\n').iter().chain(chars...)
gets rid of the additional allocation "\n".to_string()
below.
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.
We should optimize the examples for legibility, not for performance
@birkenfeld
Since this macro is mainly for testing, I think unwrapping is OK and more convenient. |
And I rewrote |
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.
Left one comment, otherwise it's looking fine
src/lib.rs
Outdated
pyo3::py_run_impl!($py, $($val)+, pyo3::indoc::indoc!($code)) | ||
}}; | ||
($py:expr, $($val:ident)+, $code:expr) => {{ | ||
pyo3::py_run_impl!($py, $($val)+, &pyo3::_indoc_runtime($code)) |
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.
Could you explain why we're sometimes using indoc and sometimes don't?
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.
indoc only accepts string literals so it doesn't work for the cases like
let code = generates_code();
py_run!(py, obj, code)
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.
Then you can use unindent, which is what indoc uses internally.
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.
OK, I'll try that
Currently, we have a macro named
py_run
in tests/common.rs.I think this macro is useful for users (e.g. when testing libraries).
When exposing this macro, I made a modification.
When string literal is passed, we use indoc to generate an indented string in compile time.
In other cases, we do string conversion in runtime.