[Pigeon] Condenses serialization formats#2745
[Pigeon] Condenses serialization formats#2745auto-submit[bot] merged 72 commits intoflutter:mainfrom
Conversation
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I gave this a quick look and it's all in the right place. One thing to make sure is that there is an explicit defined order of the fields when they are written. Writing them in the order they show up in the pigeon file is good. I'd just make sure that's the case.
packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/AsyncHandlersTest.swift
Outdated
Show resolved
Hide resolved
- Fix API call typo. - Fix double nesting of arrays for class serialization. - Replace incorrect emplace calls with push_back. - Start to update unit tests.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
C++ and Obj-C reviews done; again just small things. Swift and Kotlin tomorrow :)
| indent.scoped('{', '}', () { | ||
| indent.writeln( | ||
| 'wrapped.emplace(flutter::EncodableValue("${Keys.error}"), WrapError("$argName unexpectedly null."));'); | ||
| 'wrapped = WrapError("$argName unexpectedly null.");'); |
There was a problem hiding this comment.
Note for future PR work: this is something we should remove, and replace with a debug-only assert, since per previous offline discussion we don't want non-debug runtime checks that the wire format is correct. Could you see if we have a bug about removing runtime type/nullability checks in our deserialization across any generators that have them, and file it if not?
There was a problem hiding this comment.
Issue filed, feel free to add more detail if you think my description is lacking. flutter/flutter#116972
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
All languages complete! Other than the Kotlin wrapError thing it's all very minor.
I'm going to LGTM with these comments since I don't think anything here should need re-review, but if there's anything you change that you want me to look at let me know.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Wowee, there is a lot more to review now. 2 big things, the rest you can ignore:
- I think you have a bug in the c++ generator
- I think we should avoid building wrapped values by using
.addmethods, instead indexing exactly where the values should go.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM! Thanks for tackling this, a brave new world for pigeon.
tarrinneal
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/packages, pr: 2745, due to - The status or check suite ios-build_all_packages CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label. |
tarrinneal
added
the
autosubmit
3 participants
Condenses the serialization platform of generated api's by converting objects to lists
List which issues are fixed by this PR. You must list at least one issue.
fixes flutter/flutter#111503
fixes flutter/flutter#111722
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
tbc
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.