MAINT/TST: fix Slerp typing and better iv in Rotation
#18204
Conversation
cgobat
changed the title
Rotation and Slerp attributes and error messagesRotation and Slerp attributes and error messages
| if rotations.single or len(rotations) == 1: | ||
| raise ValueError("`rotations` must be a sequence of at least 2 rotations.") |
There was a problem hiding this comment.
I did not check but we usually have tests checking the actually message from raises. If this does not trigger anything, that would be a good sign showing a test could and should be added.
There was a problem hiding this comment.
Okay, gotcha. I just combined two checks/errors that were doing the same thing for the sake of avoiding repetition, but you are correct: this does fail the tests for that reason. I will push another commit that changes the match pattern in test_rotation.py.
There was a problem hiding this comment.
I know, and going through the codebase it might be very tempting to do small adjustments here and there. We generally tend to avoid changes which are not necessary or just cosmetic for a few reasons. The first being avoiding any risk related to a change, the second to preserve a useful git blame/history. Sometimes we allow ourselves to adjust things, and this could be ok. I (unless someone else beat me to it) will look more closely.
There was a problem hiding this comment.
Actually if we want to really improve the input validation here, adding a check on the type of rotations would be valuable I think. (This would also need a test)
tupui
added
enhancement
| if rotations.single or len(rotations) == 1: | ||
| raise ValueError("`rotations` must be a sequence of at least 2 rotations.") |
There was a problem hiding this comment.
Actually if we want to really improve the input validation here, adding a check on the type of rotations would be valuable I think. (This would also need a test)
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
|
@tupui looks like everything passes, including with the two new tests I added. Any further changes you want to see? |
tupui
changed the title
Rotation and Slerp attributes and error messagesSlerp typing and better iv in Rotation
Reference issue
N/A
What does this implement/fix?
Slerp.times,Slerp.rotations, andSlerp.timedeltaattributes.ChangesSlerp.rotvecsto be a property rather than an attribute.Rotation.__init__()andSlerp.__init__().