-
Notifications
You must be signed in to change notification settings - Fork 760
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
Add rename_all
attribute to #[pyclass]
#3384
Conversation
I'll try to update the |
4f41e79
to
928c3f9
Compare
928c3f9
to
f02fe94
Compare
@adamreichold Ready for another review. |
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.
Thank you for going the extra mile and making this fully generic! I think this is now a powerful helper that can save quite a bit of boiler plate for many people.
codecov is failing again... I think I added enough tests and it can be ignored right? |
Yes, I think so too. The only reason I did not merge this yet is to give the other maintainers some time to chime in on this version of the 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.
Looks great to me, thanks for the solid implementation.
I just have one suggestion on an addition to the test suite (which will drive the coverage just a teeny bit higher too), otherwise I'm happy to see this merged whenever.
use pyo3::types::PyBool; | ||
|
||
Python::with_gil(|py| { | ||
let struct_class = py.get_type::<StructWithRenamedFields>(); | ||
let struct_obj = struct_class.call0().unwrap(); | ||
assert!(struct_obj | ||
.setattr("firstField", PyBool::new(py, false)) | ||
.is_ok()); | ||
py_assert!(py, struct_obj, "struct_obj.firstField == False"); | ||
py_assert!(py, struct_obj, "struct_obj.secondField == 5"); | ||
assert!(struct_obj | ||
.setattr("third_field", PyBool::new(py, true)) | ||
.is_ok()); | ||
py_assert!(py, struct_obj, "struct_obj.third_field == True"); | ||
}); |
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 would be quite nice to have some kind of macro here which can test each type of rename_all in turn, so that we test they all work as expected.
test_case!(CamelCase, "camelCase", "ab_cd-ef", "abCdEf");
test_case!(SnakeCase, "snake_case", "ab_cd-ef", "ab_cd_ef");
etc
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 added a macro which I think does the job. At least codecov likes it.
Closes #3383