Rust API for pyinitconfig#6173
Conversation
2a9f29f to
2f0308c
Compare
|
According to codecov almost none of the API surface is tested. Can you please make sure there's coverage for all the |
590f4a5 to
57b35c8
Compare
| /// | ||
| /// # Panic | ||
| /// Panics if the interpreter is already initialized. | ||
| pub fn initialize(self) -> Result<Option<c_int>, InitConfigError> { |
There was a problem hiding this comment.
I think it is more appropriate to return the exit code as a variant of InitConfigError. And make this return Result<(), InitConfigError>
There was a problem hiding this comment.
I think it is more appropriate to return the exit code as a variant of
InitConfigError. And make this returnResult<(), InitConfigError>
I think you're right, I've double checked the docs, and it doesn't explicitly say that calling python APIs when the interpreter wants to exit is forbidden, but that is what the examples show.
There was a problem hiding this comment.
I think it should be a new error type with two variants, one of which wraps the existing error type. This way users who want to handle the exit code differently (i.e. by exiting with that code) don't need to check it at every call.
There was a problem hiding this comment.
I think it should be a new error type with two variants, one of which wraps the existing error type. This way users who want to handle the exit code differently (i.e. by exiting with that code) don't need to check it at every call.
Agreed.
Should InitConfigError be just a CString/String? It feels like a unnecessary wrapper. What do you think?
There was a problem hiding this comment.
I think that libraries should define their own error types. https://doc.rust-lang.org/rust-by-example/error/multiple_error_types/define_error_type.html
Merging this PR will degrade performance by 25.83%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | into_u128_u64_max |
1.5 µs | 2 µs | -25.83% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Person-93:pyinitconfig (f8416ab) with main (9e6697d)
|
@codspeedbot fix this regression |
|
Maintainers (@ngoldbaum), is test coverage OK. Do we need a test case which deliberately causes initialization to fail so the error handling code is covered? |
Adds a rust API to initialize the embedded interpreter with configuration.
Closes #6153