[dart][dart-dio] Use built_value collection types without string replacement #8153

kuhnroyal · 2020-12-10T17:43:57Z

Do not use String replacement for built_value collection types.
This fixes #4865 but also changes return/parameter types and would be a breaking change.
I think this is the correct thing to do but the changed parameter types may reduce usability in cases of list body/parameters.

@josh-burton Did you intentionally prevent the return/parameter types from being built_value types? What is your opinion on this?

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
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/configs/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.

CC @ircecho (2017/07) @swipesight (2018/09) @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12)

josh-burton · 2020-12-10T17:59:36Z

@kuhnroyal no not intentional, more a case of not knowing how to correctly substitute the built collection types. Even though a breaking change I think it's the right change. Nice work :)

kuhnroyal · 2020-12-10T18:34:25Z

@wing328 Is master the correct branch for a breaking change? How should this be documented?

agilob · 2020-12-10T20:59:45Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/DartClientCodegen.java

        importMapping.put("Set", "dart:core");
        importMapping.put("DateTime", "dart:core");

+        defaultIncludes = new HashSet<>(Collections.singletonList("dart:core"));


Sets.newHashSet from guava

agilob · 2020-12-10T21:11:53Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/DartClientCodegen.java

    }

    @Override
    public String getTypeDeclaration(Schema p) {


can you add unit tests to this method?

There are tests for dart and dart-dio that test the type returned for array and map OAS types.

agilob · 2020-12-10T21:12:33Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/DartClientCodegen.java


+    @Override
+    public CodegenOperation fromOperation(String path, String httpMethod, Operation operation, List<Server> servers) {
+        final CodegenOperation op = super.fromOperation(path, httpMethod, operation, servers);


can you add unit tests for this method? is it worth to make it more dart-dio specific as the rest of the changes are dart-dio?

Could possibly be moved to the dart-dio generator.

makes sense to keep it here for all darts.

agilob · 2020-12-10T21:18:46Z

I'm not a fan of these changes TBH, it has the following disadvantages I see straight away:

Breaking change in terms of input/output to methods
3rd party library to support such basic concepts like lists and maps
I heave to know how to use that 3rd party library to use lists/maps (I was not aware this library even exists until now)
When I generate an Owner with Pets I want to add and remove pets from existing array as I go and not be limited by immutable arrays that it holds by default. It restricts how classes can be used.
TypeScript and Java generators that I use produce mutable lists so it's also introducing incompatibility between languages and how generated code can be used
It takes dart-dio generated code further from dart generator, making them less and less compatible. TypeScript is suffering from similar problems, each generator is completely different, has different codes, settings and produces different outputs. Java and Python generators are all similar, you can swap gson with jackson generator and almost no difference in use. We should be aiming for less split between generators, not more.

If anything, this should be a runtime flag in custom properties.

That said, I much prefer shelf and not planing to use dart-dio, so whatever floats your boat works for me, but please don't put it in main dart. If these changes landed in any generator I use I would be really pissed (as a user) as it completely breaks 100% of cases how I use generated classes in Java and Dart. Autogenerated code are my DTOs and small cached objects, so shared schema and how classes are used between languages absolutely has to be the same, eg. lists and maps have to be mutable.

#4865 should be solved in a way that's compatible with other generators, so contributors from scala or python can come to here and be familiars with the generation process.

agilob · 2020-12-10T21:38:55Z

Any opinion on this @sbu-WBT ?

kuhnroyal · 2020-12-10T22:48:53Z

When I generate an Owner with Pets I want to add and remove pets from existing array as I go and not be limited by immutable arrays that it holds by default. It restricts how classes can be used.

IMO this depends on the language/framework features. I usually prefer immutability. Many other generators for languages that have immutable collections by default do the same. Java and Typescript are not some of those as we all know :)

If you choose to use this generator, you also choose to use built_value, all the models you get from the API are immutable built_value classes, everything you put into it as well. It only makes sense to use the immutable data structures provided by built_value which use the same builder pattern as the model classes. It is much easier for responses which are deserialized as BuiltList/Map/Set anyways.

but please don't put it in main dart

I would never dream of it. This is here for dart-dio because the used collection classes are part of the same serialization package that is used.

It takes dart-dio generated code further from dart generator, making them less and less compatible. TypeScript is suffering from similar problems, each generator is completely different, has different codes, settings and produces different outputs. Java and Python generators are all similar, you can swap gson with jackson generator and almost no difference in use. We should be aiming for less split between generators, not more.

Sounds great and would very much be possible if we could use dart mirrors in Flutter, like we use reflection in Java :) Languages that don't have a form reflection will have the problem of generators being a lot more different to each other because everything that is different has to be expressed in code and can not be weaved in at runtime.

It is a great idea to provide a flag to choose a specific serialization framework, as other generators do. But dart-dio doesn't do this yet and it is way out of scope for this issue.

agilob · 2020-12-10T23:02:58Z

Sounds great and would very much be possible if we could use dart mirrors in Flutter, like we use reflection in Java :) Languages that don't have a form reflection will have the problem of generators being a lot more different to each other because everything that is different has to be expressed in code and can not be weaved in at runtime.

You can swap gson with jackson generator and almost no difference in use

I didn't mean reflection or mirrors, just content of X1Generator and X2Generator and .mustache files should be as similar as possible to reduce duplication, produce outputs that are more similar. You can swap gson for jackson and as long as there are correct dependencies, it will all work without any fuss.

But as above, I'm not familiar with dart-dio, so whatever works for you here.

agilob

Minor change requests not worth effort of a commit, good to go.

[dart-dio] Use built_value collection types without string replacement

b86d5c4

josh-burton approved these changes Dec 10, 2020

View reviewed changes

kuhnroyal marked this pull request as ready for review December 10, 2020 18:28

kuhnroyal changed the title ~~[dart-dio] Use built_value collection types without string replacement~~ [dart][dart-dio] Use built_value collection types without string replacement Dec 10, 2020

agilob reviewed Dec 10, 2020

View reviewed changes

agilob approved these changes Dec 10, 2020

View reviewed changes

wing328 added Breaking change (without fallback) Client: Dart Issue: Bug labels Dec 11, 2020

wing328 added this to the 5.0.0 milestone Dec 11, 2020

wing328 merged commit 7f9012c into OpenAPITools:master Dec 13, 2020

kuhnroyal deleted the dart-dio/built_value-collections branch December 13, 2020 16:14

Uh oh!

[dart][dart-dio] Use built_value collection types without string replacement #8153

[dart][dart-dio] Use built_value collection types without string replacement #8153

Uh oh!

Conversation

kuhnroyal commented Dec 10, 2020 • edited by wing328 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

josh-burton commented Dec 10, 2020

Uh oh!

kuhnroyal commented Dec 10, 2020

Uh oh!

agilob Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

agilob Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

kuhnroyal Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

agilob Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

kuhnroyal Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

agilob Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

agilob commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilob commented Dec 10, 2020

Uh oh!

kuhnroyal commented Dec 10, 2020

Uh oh!

agilob commented Dec 10, 2020

Uh oh!

agilob left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kuhnroyal commented Dec 10, 2020 •

edited by wing328

Loading

agilob commented Dec 10, 2020 •

edited

Loading