Skip to content

Minor revise of #22#27

Merged
termoshtt merged 5 commits intomainfrom
pr/LockedThread/22
May 24, 2025
Merged

Minor revise of #22#27
termoshtt merged 5 commits intomainfrom
pr/LockedThread/22

Conversation

@termoshtt
Copy link
Copy Markdown
Member

No description provided.

@termoshtt termoshtt force-pushed the pr/LockedThread/22 branch from 2d070d3 to 53c6dcb Compare May 24, 2025 07:05
@termoshtt termoshtt requested a review from Copilot May 24, 2025 07:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes a minor revision for issue #22 by adding a new test to verify Python object conversions and enhancing the deserialization logic.

  • Added a new test (check_python_object) in tests/check_revertible.rs to validate Python and Rust object parity.
  • Updated the deserialization logic in src/de.rs by handling objects with dict and slots.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/check_revertible.rs Added a new test to verify proper conversion between Python and Rust objects.
src/de.rs Enhanced deserialization to support dict and slots attributes in Python.

Comment on lines +139 to +149
#[derive(Debug, PartialEq, Serialize, Deserialize)]
struct MyClass {
name: String,
age: i32,
}

Python::with_gil(|py| {
// Create an instance of Python object
py.run(
c_str!(
r#"
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The MyClass definition is redefined within this test. Consider extracting it to a shared test module if it's used in multiple tests to avoid duplication.

Suggested change
#[derive(Debug, PartialEq, Serialize, Deserialize)]
struct MyClass {
name: String,
age: i32,
}
Python::with_gil(|py| {
// Create an instance of Python object
py.run(
c_str!(
r#"
#[derive(Debug, PartialEq, Serialize, Deserialize)]
struct MyClass {
name: String,
age: i32,
}

Copilot uses AI. Check for mistakes.
for key in obj.getattr("__slots__")?.try_iter()? {
let key = key?;
keys.push(key.clone());
let v = obj.getattr(key.str()?)?;
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider providing additional context in error handling for key conversion (i.e., key.str()?) to account for unexpected non-string slot names, improving overall robustness.

Suggested change
let v = obj.getattr(key.str()?)?;
let key_str = key.str().map_err(|e| {
Error::custom(format!(
"Failed to convert slot name to string: {:?}, error: {}",
key, e
))
})?;
let v = obj.getattr(key_str)?;

Copilot uses AI. Check for mistakes.
@termoshtt termoshtt marked this pull request as ready for review May 24, 2025 07:13
@termoshtt termoshtt merged commit 717c0c4 into main May 24, 2025
5 checks passed
@termoshtt termoshtt deleted the pr/LockedThread/22 branch May 24, 2025 07:13
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.

4 participants