-
Notifications
You must be signed in to change notification settings - Fork 52
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
DOCS-2313: Fix docstrings in arm and encoder where link not parsing #641
DOCS-2313: Fix docstrings in arm and encoder where link not parsing #641
Conversation
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
|
||
:: | ||
|
||
my_encoder = Encoder.from_robot(robot=robot, name='my_encoder') | ||
|
||
# Get the position of the encoder in ticks | ||
position = await my_encoder.get_position(encoder.PositionTypeTicks) |
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.
LMK if this is wrong as far as I can tell new way is way to do it?
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.
One small thing that's a technicality and honestly I'd be ok not implementing it because it's just confusing overall and I think most developers will understand
print("The encoder position is currently ", position[0], position[1]) | ||
|
||
Args: | ||
position_type (PositionType.ValueType): The desired output type of the position. |
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.
This technically needs to be PositionType.ValueType
because of the way protobuf implements these types of enumerations. If this links, then it should link to the .ValueType
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.
Ahh ok-- the link wasn't being generated by the parser. I'll try adding it manually?
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.
Ohh interesting. Ok for linking purposes I think we can keep this as just PositionType
then. The actual underlying type is a property of PositionType
, so it should be straightforward. And since the actual method signature has the correct type, typechecking should work just fine.
Up to y'all as this only affects docs.
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.
That's good with me, I figured it was straightforward as well
@sguequierre lgtm if you've confirmed this fixes the issue and renders correctly |
yup, renders correctly changed to PositionType as Naveed suggested! |
Some final fixups from this ticket