Skip to content
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

Tickets/dm 45817 #221

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Tickets/dm 45817 #221

merged 3 commits into from
Oct 3, 2024

Conversation

dsanmartim
Copy link
Contributor

Hi @tribeiro

I implemented this in a way that maintains backward compatibility, ensuring the script continues to work in the same way in existing json blocks.

@@ -78,6 +78,18 @@ def get_schema(cls):
v:
type: number
description: Ry offset (deg).
reset_axis:
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming here is backwards, reset_only should be the boolean and reset_axis the name of the axis you want to reset.

Copy link
Contributor Author

@dsanmartim dsanmartim Sep 27, 2024

Choose a reason for hiding this comment

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

For the reset_only property, I focused on giving flexibility to reset any axis the user may want, instead of resetting all of them (which would be the case if this is a boolean). For the reset_axis (to reset axis before doing offset), maybe this property should be called reset_first to be more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the code to have only the reset_axis property to deal with all possible scenarios to reset the axes. Thanks for your suggestions!

@dsanmartim dsanmartim force-pushed the tickets/DM-45817 branch 5 times, most recently from 4f4fa12 to a945cc9 Compare September 29, 2024 00:13
Copy link
Member

@tribeiro tribeiro left a comment

Choose a reason for hiding this comment

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

Ok, I only have some minor typos comments in the news fragments. Everything else looks ok.

@@ -0,0 +1,3 @@
In ``offset_camera_hexapod.py``and ``offset_m2_hexapod.py``:
Copy link
Member

Choose a reason for hiding this comment

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

need a space between ```offset_camera_hexapod.py``and`

@@ -0,0 +1,3 @@
In ``offset_camera_hexapod.py``and ``offset_m2_hexapod.py``:
- add an option to reset hexapod offsets to zero. In this case, it only reset offset and don't do anything else.
Copy link
Member

Choose a reason for hiding this comment

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

Please, use semantic line breaking and capitalize the "a" in "Add"

@@ -0,0 +1,3 @@
In ``offset_camera_hexapod.py``and ``offset_m2_hexapod.py``:
- add an option to reset hexapod offsets to zero. In this case, it only reset offset and don't do anything else.
- add an option to reset hexapod offsets before applying new offsets. In this case, it reset offset before applying new offsets.
Copy link
Member

Choose a reason for hiding this comment

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

Please, use semantic line breaking and capitalize the "a" in "Add"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tks. The messages were really bad actually. I just improved them, taking your suggestions into account.

@dsanmartim dsanmartim force-pushed the tickets/DM-45817 branch 2 times, most recently from 5f6e5e4 to a04856e Compare October 2, 2024 22:30
@dsanmartim dsanmartim merged commit 65fd9b9 into develop Oct 3, 2024
2 checks passed
@dsanmartim dsanmartim deleted the tickets/DM-45817 branch October 3, 2024 03:07
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.

2 participants