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

[Dart] Fix enum generation #6729

Merged
merged 26 commits into from
Jul 23, 2020
Merged

[Dart] Fix enum generation #6729

merged 26 commits into from
Jul 23, 2020

Conversation

agilob
Copy link
Contributor

@agilob agilob commented Jun 20, 2020

  1. Correctly generates Enums
  2. minor - fix indentation for mapListFromJson

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@ircecho @swipesight @jaumard @athornz @amondnet

fixes #6727 #4974

Also fixes #3633 by removing default initialization to null.

@agilob
Copy link
Contributor Author

agilob commented Jun 21, 2020

Dart doesnt support inner classes and test is failing because enum StatusEnum is defined twice in pet.dart and order.dart.

Easy solution to this would be to generate enum name as {{classname}}{{datatypeWithEnum}} so it generates PetStatusEnum. Scala-lagom and nodejs-grapqhl is using it, so I'm not first getting this problem. Considering this also isn't a reusable enum in dart, but class specific, I'm going to go this direction.

@agilob
Copy link
Contributor Author

agilob commented Jun 21, 2020

Imo this is ready now, @ircecho (2017/07) @swipesight (2018/09) @jaumard (2018/09) @nickmeinhold (2019/09) @athornz (2019/12) @amondnet (2019/12)

@wing328 would it be possible to get this in the next release 5? Is there any specific roadmap?

@wing328
Copy link
Member

wing328 commented Jun 24, 2020

@wing328 would it be possible to get this in the next release 5? Is there any specific roadmap?

Yes, I think so. We'll release 5.0.0-beta to start with (hopefully this week).

For the roadmap, please refer to https://github.com/OpenAPITools/openapi-generator/blob/master/docs/roadmap.md

@agilob
Copy link
Contributor Author

agilob commented Jun 28, 2020

@wing328 Fixed conflicts

@agilob agilob marked this pull request as draft June 28, 2020 08:40
@agilob
Copy link
Contributor Author

agilob commented Jun 28, 2020

Found a regression when non-required fields with default value aren't correctly generated.

@@ -414,7 +416,7 @@ public String toDefaultValue(Schema schema) {
}
return schema.getDefault().toString();
} else {
return "null";
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes not setting null value on properties that don't have default value. Dart is evolving to non-null by default language, so this will be important in the next releases of the language.

@agilob agilob marked this pull request as ready for review June 28, 2020 17:29
@agilob
Copy link
Contributor Author

agilob commented Jun 28, 2020

@wing328 opening back for review after correctly setting default value and removing null from default values.

https://dart.dev/null-safety

@agilob

This comment has been minimized.

@agilob
Copy link
Contributor Author

agilob commented Jul 2, 2020

@wing328 can we have this merged please?

@agilob agilob requested a review from wing328 July 14, 2020 11:15
@sbu-WBT
Copy link
Contributor

sbu-WBT commented Jul 15, 2020

Hi, I tested this PR with this https://github.com/geoDavey/osrm-openapi/blob/master/osrm-openapi.yaml OpenAPI specification.
I'm get still something like this ApiResponseCodeEnum code = ;.
I'm also getting some invalid enum classes:

class IntersectionList<EntryEnum> {
  /// The underlying value of this enum member.
  final List<String> value;

  const IntersectionList<EntryEnum>._internal(this.value);

  static const IntersectionList<EntryEnum> true_ = IntersectionList<EntryEnum>._internal("true");
  static const IntersectionList<EntryEnum> false_ = IntersectionList<EntryEnum>._internal("false");

  List<String> toJson () {
    return value;
  }

  @override
  String toString () {
    return value;
  }

  static IntersectionList<EntryEnum> fromJson(List<String> value) {
    return IntersectionList<EntryEnum>TypeTransformer().decode(value);
  }

  static List<IntersectionList&lt;EntryEnum&gt;> listFromJson(List<dynamic> json) {
    return json == null
        ? List<IntersectionList&lt;EntryEnum&gt;>()
        : json.map((value) => IntersectionList&lt;EntryEnum&gt;.fromJson(value)).toList();
  }
}

class IntersectionList&lt;EntryEnum&gt;TypeTransformer {

  dynamic encode(IntersectionList&lt;EntryEnum&gt; data) {
    return data.value;
  }

  IntersectionList&lt;EntryEnum&gt; decode(dynamic data) {
    switch (data) {
      case "true": return IntersectionList&lt;EntryEnum&gt;.true_;
      case "false": return IntersectionList&lt;EntryEnum&gt;.false_;
      default: return null;
    }
  }
}

Could you have a look over it why it happens? Because some enum classes generation works.
Thanks

@agilob
Copy link
Contributor Author

agilob commented Jul 22, 2020

I'm get still something like this ApiResponseCodeEnum code = ;.

That's because allOf and anyOf aren't supported, and it worked (by letting code compiled by defaulting to null) by accident. reverted the breaking change.

@wing328
Copy link
Member

wing328 commented Jul 23, 2020

Tested again and the result looks good.

@wing328 wing328 merged commit 90d8c32 into OpenAPITools:master Jul 23, 2020
@wing328 wing328 added this to the 5.0.0 milestone Aug 9, 2020
@agilob agilob deleted the 6727 branch September 25, 2021 20:35
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.

[BUG][Dart] Generated Enum is a string without validation [REQ] [Dart] Don't initialize variables to null
4 participants