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

Introduce bytes type #1543

Merged
merged 4 commits into from
May 23, 2023
Merged

Introduce bytes type #1543

merged 4 commits into from
May 23, 2023

Conversation

heinrich5991
Copy link
Contributor

@heinrich5991 heinrich5991 commented May 16, 2023 •
edited
Loading

This type is mapped to the native bytestring type in the various languages, i.e.

  • Kotlin: ByteArray
  • Python: bytes
  • Ruby: String with Encoding::BINARY
  • Swift: Data

Also add tests exercising this new type.

Fixes #1299.
Fixes #1541.

@heinrich5991
Copy link
Contributor Author

Where/how would I add tests?

This is just a draft so far, Python works, the rest of the languages aren't implemented yet. What do you think about the current approach?

CC #1299 #1541

mhammond
mhammond reviewed May 17, 2023
View reviewed changes
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This looks like a good start. I think I prefer just "bytes" instead of "bytestring" though. For tests you should look at, say, fixtures/coverall and add a "bytes" object to each of the types supported, and as results and args to methods/functions etc, then each language should test them too.

@heinrich5991 heinrich5991 changed the title Introduce bytestring type Introduce bytes type May 19, 2023
@heinrich5991 heinrich5991 marked this pull request as ready for review May 19, 2023 23:31
@heinrich5991 heinrich5991 requested a review from a team as a code owner May 19, 2023 23:31
@heinrich5991 heinrich5991 requested review from mhammond and removed request for a team May 19, 2023 23:31
@heinrich5991
Copy link
Contributor Author

@mhammond I added support for Kotlin, Ruby and Swift and added tests. For Kotlin, I wasn't sure whether to use the UByteArray or the ByteArray type, UByteArray seems newer and not as well integrated as ByteArray.

The PR should be ready for review now.

badboy
badboy reviewed May 22, 2023
View reviewed changes
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

I haven't given this a full review yet, but will do that asap.

fixtures/coverall/tests/bindings/test_coverall.kts Show resolved Hide resolved
fixtures/coverall/tests/test_generated_bindings.rs Outdated Show resolved Hide resolved
@heinrich5991 heinrich5991 force-pushed the pr_bytes branch 2 times, most recently from 85c71af to 2f5cbd2 Compare May 22, 2023 10:17
@heinrich5991
Copy link
Contributor Author

I forgot to add the bytes tests in the dictionary tests in Kotlin. Do you know why I have to use contentEquals instead of ==? I thought Kotlin implements structural equality for ==: https://kotlinlang.org/docs/equality.html.

@badboy
Copy link
Member

badboy commented May 22, 2023

contentEquals

Because == is just a.equals(b) and equals is not overriden for those collections.

badboy
badboy approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Some minimal nits regarding the tests, but otherwise this should be good to go.

fixtures/coverall/src/lib.rs Outdated Show resolved Hide resolved
fixtures/coverall/src/lib.rs Outdated Show resolved Hide resolved
@heinrich5991
Add createNoneDict to the tests of Swift and Kotlin
1873d60
@heinrich5991
Introduce bytes type
e5c8f75 
This type is mapped to the native bytestring type in the various
languages, i.e.

- Kotlin: `UByteArray`
- Python: `bytes`
- Ruby: `String` with `Encoding::BINARY`
- Swift: `Data`

Also add tests exercising this new type.

Fixes mozilla#1299.
Fixes mozilla#1541.
@heinrich5991
Change Kotlin's bytes type from UByteArray to ByteArray
8d82ffb 
This reflects the fact that `ByteArray` is a lot more common than
`UByteArray`.
@heinrich5991
Copy link
Contributor Author

Done.

@heinrich5991
Copy link
Contributor Author

Test failure seems unrelated.

mhammond
mhammond reviewed May 23, 2023
View reviewed changes
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Fantastic! It looks great, and sorry I didn't mention this before, but this really needs a changelog entry and a change to the docs

@heinrich5991
Copy link
Contributor Author

this really needs a changelog entry and a change to the docs

Done, I think. Did I find all the relevant places?

mhammond
mhammond approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Thanks!

@mhammond mhammond merged commit f243005 into mozilla:main May 23, 2023
@StuartHarris StuartHarris mentioned this pull request Jun 30, 2023
Merged
@Sajjon Sajjon mentioned this pull request Feb 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linabutler linabutler linabutler left review comments

@mhammond mhammond mhammond approved these changes

@badboy badboy badboy approved these changes

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

Successfully merging this pull request may close these issues.

Add a bytes type to UDL Python bindings: use bytes for sequence<u8>
4 participants
@heinrich5991 @badboy @mhammond @linabutler