-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[pigeon] Moves all codec logic to singular custom codec #6600
base: main
Are you sure you want to change the base?
Conversation
d43b42e
to
a175295
Compare
packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp
Outdated
Show resolved
Hide resolved
Add test to prevent duplicate entries in Kotlin generator
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 is great! It's awesome to see how much complexity and branching was removed from all over the generators.
...igeon/platform_tests/alternate_language_test_plugin/example/ios/RunnerTests/NullFieldsTest.m
Outdated
Show resolved
Hide resolved
packages/pigeon/platform_tests/alternate_language_test_plugin/ios/Classes/CoreTests.gen.h
Outdated
Show resolved
Hide resolved
...atform_tests/test_plugin/android/src/test/kotlin/com/example/test_plugin/AllDatatypesTest.kt
Show resolved
Hide resolved
packages/pigeon/lib/pigeon_lib.dart
Outdated
@@ -2249,14 +2268,16 @@ ${_argParser.usage}'''; | |||
options = options.merge(PigeonOptions( | |||
objcOptions: (options.objcOptions ?? const ObjcOptions()).merge( | |||
ObjcOptions( | |||
headerIncludePath: path.basename(options.objcHeaderOut!))))); | |||
headerIncludePath: options.objcOptions?.headerIncludePath ?? | |||
path.basename(options.objcHeaderOut!))))); |
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.
Is there a unit test that confirms this resolves flutter/flutter#147587?
When I played with it when I discovered the issue, I also had to change a few other things: flutter/flutter#147587 (comment)
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.
Still need to finish that part of the pr actually
[pigeon] Moves all codec logic to singular custom codec.
Also fixes a few small codec related bugs that have cropped up over time.
fixes flutter/flutter#147454
fixes flutter/flutter#147127
fixes flutter/flutter#147587
fixes flutter/flutter#148065