Skip to content

Conversation

@ailtonvivaz
Copy link
Contributor

This PR supports non-nullable fields of classes and enums.

Issue flutter/flutter#59118

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@ailtonvivaz ailtonvivaz force-pushed the non-null-nested-object branch from 4a46dd9 to 57782e5 Compare March 13, 2022 23:19
@stuartmorgan-g
Copy link
Collaborator

I'm confused by the description here; non-nullable field support was added in #549. What exactly is this PR doing?

@gaaclarke
Copy link
Member

I'm confused by the description here; non-nullable field support was added in #549. What exactly is this PR doing?

Looks like #549 was only tested on builtin types. Custom objects have a slightly different flow through the code and this appears to make that work.

Copy link
Member

@gaaclarke gaaclarke left a 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 ? '?' : '';
Copy link
Member

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.

Copy link
Contributor Author

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?

@ailtonvivaz
Copy link
Contributor Author

ailtonvivaz commented Mar 15, 2022

What about objc and java?

I have the follow ideia:
Instead of pass the DartOptions to objc and java generators and change every nullable condition to verify if Dart code is nullsafe, we pass the isNullSafe to _RootBuilder and assign all variables as nullable in case of Dart code not be nullsafe.
How about it?

@ailtonvivaz ailtonvivaz force-pushed the non-null-nested-object branch from 8acce59 to ec74255 Compare March 15, 2022 23:01
@gaaclarke
Copy link
Member

What about objc and java?

I have the follow ideia: Instead of pass the DartOptions to objc and java generators and change every nullable condition to verify if Dart code is nullsafe, we pass the isNullSafe to _RootBuilder and assign all variables as nullable in case of Dart code not be nullsafe. How about it?

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.

@ailtonvivaz
Copy link
Contributor Author

ailtonvivaz commented Mar 15, 2022

What about objc and java?

I have the follow ideia: Instead of pass the DartOptions to objc and java generators and change every nullable condition to verify if Dart code is nullsafe, we pass the isNullSafe to _RootBuilder and assign all variables as nullable in case of Dart code not be nullsafe. How about it?

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.
With #549, it was added the feature to non nullable fields, but looks like it doesn't cover --no-dart_null_safety.
Unless this argument be deprecated, it would be nice cover the pre nullsafe codes.

I thought: If all variables be nullable when the argument --no-dart_null_safety is passed, all code will be worked as expected because for Dart without nullsafe, String is same as String? in Dart with nullsafe

@stuartmorgan-g
Copy link
Collaborator

What's the status of this PR? Is it waiting for more review, or updates based on the discussion above?

@ailtonvivaz
Copy link
Contributor Author

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

@ailtonvivaz
Copy link
Contributor Author

Looks like the PR #1543 implements the same thing.
If was the case, this PR could be closed.

@gaaclarke
Copy link
Member

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.

@gaaclarke gaaclarke closed this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants