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

Unnecessary nullable on dynamic fields. #72

Closed
luis901101 opened this issue Dec 5, 2022 · 2 comments · Fixed by #78
Closed

Unnecessary nullable on dynamic fields. #72

luis901101 opened this issue Dec 5, 2022 · 2 comments · Fixed by #78
Labels
bug Something isn't working enhancement New feature or request

Comments

@luis901101
Copy link

luis901101 commented Dec 5, 2022

Describe the issue
Unnecessary nullable on dynamic fields.
If I have a filed like dynamic data the generated code will use dynamic? data in the _$MyClassCWProxy implementation.

Paste the package version.

dependencies:
  copy_with_extension: ^5.0.0
dev_dependencies:
  copy_with_extension_gen: ^5.0.0

To Reproduce
Steps to reproduce the behaviour:

@CopyWith(skipFields: true)
class MyClass {
  dynamic data;

  MyClass({
    this.data,
  });
}

Expected behaviour
Avoid using unnecessary ? on dynamic fields.

@numen31337 numen31337 added the enhancement New feature or request label Dec 8, 2022
@luis901101
Copy link
Author

luis901101 commented Feb 9, 2023

Another issue with dynamic fields I just faced is this:

MyClass call({
    dynamic? data,
  });

MyClass call({
    Object? data = const $CopyWithPlaceholder(),
  }) {
    return MyClass(
      data: data == const $CopyWithPlaceholder() || data == null
          // ignore: unnecessary_non_null_assertion
          ? _value.data!
          // ignore: cast_nullable_to_non_nullable
          : data as dynamic,
    );
  }

Specifically the _value.data! and the previous condition data == null, so when data is null and I use: var copyInstance = instance.copyWith(); NullPointerException throws.

I think generated code should look like:

MyClass call({
    dynamic data,
  });

MyClass call({
    Object? data = const $CopyWithPlaceholder(),
  }) {
    return MyClass(
      data: data == const $CopyWithPlaceholder()
          // ignore: unnecessary_non_null_assertion
          ? _value.data
          // ignore: cast_nullable_to_non_nullable
          : data as dynamic,
    );
  }

@numen31337
Copy link
Owner

numen31337 commented Feb 25, 2023

This comment describes crash from #74.

The initial message is another issue about this:

TestNullability call({
  int? nullableWithNonNullableConstructor,
  dynamic? dynamicField, /// <--- ISSUE, dynamic? makes no sense.
  List<int>? integers,
});

// info • test/gen_nullability_test.g.dart:25:12 • The '?' is unnecessary because 'dynamic' is nullable without it. • unnecessary_question_mark

@numen31337 numen31337 added the bug Something isn't working label Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants