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

Stabilize Dart #50

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Stabilize Dart #50

wants to merge 39 commits into from

Conversation

temeddix
Copy link

@temeddix temeddix commented Oct 4, 2024

Summary

Fixes #49

  • Added tests for Dart, just like Golang
    • Made bool check more strict
    • Fixed some parts that were reading container length as Int64, not Uint64: This wasn't a problem when the number is small(0x00000000~0x01111111), but was one of the problems that were causing the tests to fail
    • Made serialization/deserialization consider max bytes length
    • Made serialization/deserialization consider container depth
  • Implemented sortMapEntries for BCS
  • Bumped Dart versions in the CI and package config to 3.5.3(>=3.0.0, <4.0.0)

Test Plan

Since the tests were already defined in Golang, I've written some extra Dart tests that follows the way Golang does.

@temeddix temeddix requested a review from ma2bd as a code owner October 4, 2024 16:24
@temeddix temeddix marked this pull request as draft October 5, 2024 02:48
@temeddix temeddix marked this pull request as ready for review October 7, 2024 14:16
@temeddix temeddix marked this pull request as draft October 7, 2024 14:20
@temeddix temeddix marked this pull request as ready for review October 7, 2024 14:31
@temeddix temeddix marked this pull request as draft October 7, 2024 15:16
serde-generate/tests/test_utils.rs Outdated Show resolved Hide resolved
serde-generate/tests/test_utils.rs Outdated Show resolved Hide resolved
serde-generate/tests/test_utils.rs Outdated Show resolved Hide resolved
serde-generate/tests/test_utils.rs Outdated Show resolved Hide resolved
@temeddix temeddix marked this pull request as ready for review October 9, 2024 05:46
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

Thanks and congrats on the progress. Yes please would you be able to use the matcher package and keep the existing "complex map" test?

serde-generate/tests/test_utils.rs Outdated Show resolved Hide resolved
@temeddix
Copy link
Author

temeddix commented Oct 16, 2024

The matcher package doesn't support comparing lists within tuples. I might need to write a custom comparison class that resembles matcher here.

@ma2bd
Copy link
Contributor

ma2bd commented Oct 16, 2024

The matcher package doesn't support comparing list in tuple. I might need to write a custom comparison class that resembles matcher here.

Maybe we can also disable this example for Dart.

@temeddix
Copy link
Author

Okay :)

Comment on lines +203 to +208
// Exclude `SerdeData::ComplexMap` from tests
// because the `matcher` package used by the `test` package
// doesn't support lists within tuples.
if (value is SerdeDataComplexMap) {{
continue;
}}
Copy link
Author

Choose a reason for hiding this comment

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

I've disabled SerdeData::ComplexMap from the Dart tests as per your suggestion. I agree that this is the cleanest way to overcome the limitations of the matcher package.

All changes in the v13 test value were reverted.

@ma2bd
Copy link
Contributor

ma2bd commented Oct 28, 2024

Thanks @temeddix. Perhaps @jerel would like to have a look at the Dart specific code?

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.

[Feature Request] Finish the support for Dart
2 participants