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.

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

Correctly map byte[] AKA IDL type sequence<octet> to Python type bytes #142

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Draft

Correctly map byte[] AKA IDL type sequence<octet> to Python type bytes #142

wants to merge 1 commit into from

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Aug 27, 2021 •
edited
Loading

As reported in #134, the message type byte[] AKA IDL type sequence<octet> is mapped to a sequence of bytes, rather than a single bytes instance - which is the mapped type documented in the design doc.

The goal of this (draft, for now) PR is to rectify that behavior.

In 14969f2, I've modified the interface tests for Arrays, BoundedSequences, and UnboundedSequences to reflect the correct mapping from IDL type sequence<octet> to Python type bytes, rather than Python type list. As expected, these changes will cause test failures in the build job.

Signed-off-by: Abrar Rahman Protyasha [email protected]

@aprotyas
Add correct IDL sequence<octet> mapping in tests
14969f2 
The tests for `Arrays`, `BoundedSequences`, and `UnboundedSequences` are
modified to reflect the correct mapping from IDL type `sequence<octet>`
to Python type `bytes`, rather than Python type `list`.

Signed-off-by: aprotyas <[email protected]>
@aprotyas aprotyas linked an issue Dec 27, 2021 that may be closed by this pull request
byte[] AKA IDL sequence<octet> fields required to be sequence of bytes instances instead of bytes instance #134
Open
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:24
@russkel
Copy link

russkel commented Jul 18, 2023

Hi @aprotyas did this get anywhere?

@aprotyas
Copy link
Member Author

@russkel no, I didn't look into this much further than what's in the PR. Feel free to work on it if you want!

@russkel
Copy link

russkel commented Jul 18, 2023

@russkel no, I didn't look into this much further than what's in the PR. Feel free to work on it if you want!

Yeah I might have a go. Seems a good first ticket on this rosidl stuff, bit concerned about the PR sitting ignored though as has happened elsewhere.

@aprotyas
Copy link
Member Author

Yeah, unfortunately I can't give time to this PR anymore, but I'd be happy to review and move forward things if you write some code.

@gavanderhoorn
Copy link

gavanderhoorn commented Aug 14, 2024 •
edited
Loading

I haven't checked all of the related PRs and issues, but this seems like a much needed change.

It took me some time to realise byte[] is mapped onto list[bytes], which is at least counter-intuitive (it's documented, but still).

@aprotyas
Copy link
Member Author

Almost reaching its 3rd cake day! I'd be very happy if someone could take over this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quarkytale quarkytale Awaiting requested review from quarkytale quarkytale will be requested when the pull request is marked ready for review quarkytale is a code owner

@sloretz sloretz Awaiting requested review from sloretz sloretz will be requested when the pull request is marked ready for review sloretz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

byte[] AKA IDL sequence<octet> fields required to be sequence of bytes instances instead of bytes instance
3 participants
@aprotyas @russkel @gavanderhoorn