Skip to content

RSDK-4789 - Rename MoveOnGlobeNew to MoveOnGlobe #411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 12, 2023
Merged

RSDK-4789 - Rename MoveOnGlobeNew to MoveOnGlobe #411

merged 4 commits into from
Dec 12, 2023

Conversation

nicksanford
Copy link
Member

@nicksanford nicksanford commented Dec 7, 2023 •
edited
Loading

This was referenced Dec 8, 2023
Merged
Merged
@nicksanford nicksanford requested a review from raybjork December 11, 2023 16:30
raybjork
raybjork suggested changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@raybjork raybjork left a comment

Choose a reason for hiding this comment

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

Just a comment on the comment

proto/viam/service/motion/v1/motion.proto
// Generate and begin executing an execution to move a component
// to a specific GPS coordinate.
// May replan to avoid obstacles & account for location drift.
// Creates a new plan upon replanning.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a little confusing without a greater description of what these terms mean, which IMO is what our docs page is for. Maybe we should just keep it general rather than implying things about what happens under the hood?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually none of the other methods even have comments on them so I think its maybe fine to not add this? I would hope that people would refer to our docs page for usage, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

discussed offline, I believe these comments are used by some SDKs

@nicksanford nicksanford requested a review from raybjork December 12, 2023 17:41
@nicksanford nicksanford mentioned this pull request Dec 12, 2023
Merged
@nicksanford nicksanford marked this pull request as ready for review December 12, 2023 20:14
raybjork
raybjork approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@raybjork raybjork left a comment

Choose a reason for hiding this comment

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

I still think you should remove the comment. I just made RSDK-6055 to track this and think we should just do everything in this file as a part of that ticket. Either way though you have the go ahead to merge

@Simberific Simberific merged commit 18b7491 into viamrobotics:main Dec 12, 2023
@nicksanford nicksanford deleted the RSDK-4789 branch December 12, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@raybjork raybjork raybjork approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
protos-compiled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicksanford @raybjork @Simberific @viambot