andrasferenczi / dart-data-plugin

IDEA plugin for generating utility methods for Dart data classes
148 stars 31 forks source link

Null safety #19

Closed andrasferenczi closed 3 years ago

andrasferenczi commented 3 years ago

Checked out if the project still compiles and then dived deeper into it.

andrasferenczi commented 3 years ago

There is an issue in the DeclarationExtractor, which means types that are optional (have a ? at the end) are not recognized properly. It is complicated to fix, it needs to be understood how the PSIElements map to different types (and the earlier code as well).

Relevant part can be found here.

mzdm commented 3 years ago

There is an issue in the DeclarationExtractor, which means types that are optional (have a ? at the end) are not recognized properly. It is complicated to fix, it needs to be understood how the PSIElements map to different types (and the earlier code as well).

Relevant part can be found here.

Currently taking a look. "?" might not be recognized because it uses old dart dependency dart:191.6707.7' which did not support null safety at that time. If we raise the minimum IntelliJ version requirement and update Dart plugin it should work I think.

mzdm commented 3 years ago

There is an issue in the DeclarationExtractor, which means types that are optional (have a ? at the end) are not recognized properly. It is complicated to fix, it needs to be understood how the PSIElements map to different types (and the earlier code as well). Relevant part can be found here.

Currently taking a look. "?" might not be recognized because it uses old dart dependency dart:191.6707.7' which did not support null safety at that time. If we raise the minimum IntelliJ version requirement and update Dart plugin it should work I think.

yup - I tried on IntelliJ 2019.1.1 and it gives: java.lang.RuntimeException: No type is available - this variable should not be assignable from constructor

on 2020.2 works fine

Example code

class Model {
  final String? str;
}

However, it adds "?" 2 times (in copyWith)

```dart class Model { final String? str; // const Model({ this.str, }); @override bool operator ==(Object other) => identical(this, other) || (other is Model && runtimeType == other.runtimeType && str == other.str); @override int get hashCode => str.hashCode; @override String toString() { return 'Model{' + ' str: $str,' + '}'; } Model copyWith({ String?? str, }) { return new Model( str: str ?? this.str, ); } Map toMap() { return { 'str': this.str, }; } factory Model.fromMap(Map map) { return new Model( str: map['str'] as String?, ); } // } ```
andrasferenczi commented 3 years ago

@mzdm

Currently taking a look. "?" might not be recognized because it uses old dart dependency dart:191.6707.7' which did not support null safety at that time.

I was thinking the same thing, but then I got confused a bit when I saw in the PSIViewer that the elements are still there. I think that I am also using a newer version of the Dart plugin, maybe that's why it was shown?

About bumping up the version: the issue is that Android Studio relies on a somewhat older version of IntelliJ, so adding the 2020.2 as a dependency would prevent AS users from using it in the next months. How do you go about finding matching IntelliJ version and Dart plugin versions? I've encountered too many times that simply pasting a version from the available Dart plugins caused build errors, because they were not found.

(I guess we are talking about this part in gradle)

intellij {
    version '2019.1.1'
    plugins = ['dart:191.6707.7']
    pluginName 'Dart Data class methods generator'
    intellij.updateSinceUntilBuild false
}

on 2020.2 works fine ... However, it adds "?" 2 times (in copyWith)

That is major progress right here!

What needs to be done from here is

Could you share how you did that with the versioning?

mzdm commented 3 years ago

Could you share how you did that with the versioning?

gradlew buildPlugin and imported manually .jar file, I did not change any version.

...

Now thinking more about it - the dart dependency actually does not get packed with the built .jar plugin (the size would be much bigger) - that led me to think that it's probably version to just development to access the APIs and it is probably a minimal version.

That means that if you're on a newer version of IntelliJ you're using a more up-to-date Dart dependency which has support for null safety in AST Node. I'm a beginner in this, so I'm just guessing.

Btw. 2020.2 = 202 build on Android Studio and I was actually testing that code in Android Studio (and now went out 2021 AS version) so null safety is supported there as well.

Dart versions and its compatibility is listed here https://plugins.jetbrains.com/plugin/6351-dart/versions - so for example version 202.8070 would be ok (2020.2.1 and higher). It's just a version for development, each user has its own version based on the IntelliJ version. (this dependency stuff are really confusing for me here, so hopefully, it's like that :D)

So to sum up:

mzdm commented 3 years ago

What needs to be done from here is

  • remove the ? from the type
  • add a modifier for nullable variable
  • change a couple templates

I'll take a look, also make required keyword work for non-nullable types if null safety is enabled

andrasferenczi commented 3 years ago

this dependency stuff are really confusing for me here, so hopefully, it's like that

Agreed. This time it worked easily though, just picking a Dart plugin and IntelliJ version and it did not throw errors. Guess I was just lucky.

andrasferenczi commented 3 years ago

We are really close to release guys!

I haven't coded in Dart for a long time, can someone help me how to rewrite these arguments into a nullable type:


  factory Person.fromMap(
    Map<String, dynamic> map, {
    String keyMapper(String key)
  }) {
    keyMapper ??= (key) => key;

    return Person(
      id: map[keyMapper('id')] as int,
      firstName: map[keyMapper('_firstName')] as String,
      lastName: map[keyMapper('_lastName')] as String,
      age: map[keyMapper('age')] as int,
      income: map[keyMapper('income')] as int,
    );
  }

I have no idea how to make the keyMapper optional so if anyone could help out, it would be great.

And some testing would be nice as well: I just played around with a Person class, but I don't know if that covers all cases.

mzdm commented 3 years ago

We are really close to release guys!

I haven't coded in Dart for a long time, can someone help me how to rewrite these arguments into a nullable type:

  factory Person.fromMap(
    Map<String, dynamic> map, {
    String keyMapper(String key)
  }) {
    keyMapper ??= (key) => key;

    return Person(
      id: map[keyMapper('id')] as int,
      firstName: map[keyMapper('_firstName')] as String,
      lastName: map[keyMapper('_lastName')] as String,
      age: map[keyMapper('age')] as int,
      income: map[keyMapper('income')] as int,
    );
  }

I have no idea how to make the keyMapper optional so if anyone could help out, it would be great.

And some testing would be nice as well: I just played around with a Person class, but I don't know if that covers all cases.

When is keyMapper used? In my generated code, I don't see it

edit: oh I see, it needs to be enabled

mzdm commented 3 years ago
  factory Person.fromMap(
    Map<String, dynamic> map, {
    String keyMapper(String key)
  }) {
    keyMapper ??= (key) => key;

    return Person(
      id: map[keyMapper('id')] as int,
      firstName: map[keyMapper('_firstName')] as String,
      lastName: map[keyMapper('_lastName')] as String,
      age: map[keyMapper('age')] as int,
      income: map[keyMapper('income')] as int,
    );
  }

I have no idea how to make the keyMapper optional so if anyone could help out, it would be great.

Maybe changing the parameter from String keyMapper(String key) to String Function(String key)? keyMapper?

I did some testing and did not encounter any other issues. Time to :shipit: ? 👀

andrasferenczi commented 3 years ago

Thank you for your help @mzdm !

Plugin is uploaded and waiting for Jetbrains' approval. Cheers!

mzdm commented 3 years ago

Thank you for your help @mzdm !

Plugin is uploaded and waiting for Jetbrains' approval. Cheers!

Thanks for keeping the plugin updated even though you don't use Dart anymore 🔥