-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[pigeon] Adds non-null object fields #1191
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
Conversation
4a46dd9 to
57782e5
Compare
|
I'm confused by the description here; non-nullable field support was added in #549. What exactly is this PR doing? |
gaaclarke
left a 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.
What about objc and java?
| 'final Map<Object$nullTag, Object$nullTag> pigeonMap = <Object$nullTag, Object$nullTag>{};', | ||
| ); | ||
| for (final NamedType field in klass.fields) { | ||
| final String nullsafe = field.type.isNullable ? '?' : ''; |
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.
Everywhere else in the code we use isNullSafe to declare operators, then switch on wether something is nullable or not. That's reversed here which is a bit harder to follow.
The name "nullsafe" isn't helpful.
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.
I've made some changes about it. Could be that?
I have the follow ideia: |
8acce59 to
ec74255
Compare
I'm not sure how that is going to get us objc and java coverage. We need to do what you did here to verify that non-null objects work correctly on the objc and java generators. |
Objc and java generators already test nullable fields. I thought: If all variables be nullable when the argument |
|
What's the status of this PR? Is it waiting for more review, or updates based on the discussion above? |
I don't know about the need of java coverage then it would be a revision if the implementation is enough |
|
Looks like the PR #1543 implements the same thing. |
|
I see. I didn't realize that the problem only existed with the Dart generator. I'll work with stuart to get his PR merged. What neither PR had was platform tests which we should have. |
This PR supports non-nullable fields of classes and enums.
Issue flutter/flutter#59118
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.///).