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

Fix length-limited list types #840

Closed

Conversation

EzraBrooks
Copy link

@EzraBrooks EzraBrooks commented Feb 24, 2023

Public API Changes

None

Description

Removes the , N length specifier at the end of list types like sequence<type, length>. Fixes issues with serializing shape_msgs/Polygon, which has a float64[<=3] field.

Fixes #821

@EzraBrooks
Copy link
Author

@achim-k can I get a review? this is a pretty brutal issue - it prevents serializing Polygons, which many planning- and geometry-related messages include.

Copy link
Contributor

@achim-k achim-k left a comment

Choose a reason for hiding this comment

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

I'm unfamiliar with the code path this PR touches but it looks correct. Could you add a unit test for this?

@achim-k
Copy link
Contributor

achim-k commented Mar 1, 2023

@EzraBrooks #827 looks like it does the same / something similar. Can you check that one?

@EzraBrooks EzraBrooks marked this pull request as draft March 6, 2023 23:28
@github-actions
Copy link

github-actions bot commented Sep 3, 2023

This PR has been marked as stale because there has been no activity in the past 6 months. Please add a comment to keep it open.

@github-actions github-actions bot added the stale label Sep 3, 2023
@JensVanhooydonck
Copy link

Got to the same solution. This solution seems ready to merge.

@EzraBrooks EzraBrooks marked this pull request as ready for review October 2, 2023 14:13
@github-actions github-actions bot removed the stale label Oct 3, 2023
@sea-bass
Copy link
Contributor

@achim-k The other PR (#827) has been closed and this one is tested and ready to merge. Can we merge this one in?

@EzraBrooks
Copy link
Author

Closing in favor of #883, which fixes this particular issue and others.

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.

AttributeError: 'float' object has no attribute 'get_fields_and_field_types'
4 participants