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.

Sign up for GitHub

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

Jump to bottom

Dart - update generator to support null safety #10637

Merged
merged 52 commits into from
Jan 2, 2022
Merged

Dart - update generator to support null safety #10637

merged 52 commits into from
Jan 2, 2022

Conversation

agilob
Copy link
Contributor

@agilob agilob commented Oct 19, 2021 •
edited
Loading

Targeting 6.x as there's plenty of breaking changes here

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.
  • Run the following to build the project and update samples: 
    ./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
    Commit all changed files.
    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 (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob @ahmednfwela

@agilob agilob marked this pull request as ready for review October 27, 2021 13:49
@agilob agilob changed the base branch from master to 6.0.x October 27, 2021 13:52
@agilob agilob mentioned this pull request Oct 27, 2021
Closed
5 tasks
@agilob
Copy link
Contributor Author

agilob commented Dec 21, 2021

@jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob @ahmednfwela @noordawod any last reviews? can I get green ticks on this PR please?

@ahmednfwela
Copy link
Contributor

note: the java changes also affect other generators

kuhnroyal
kuhnroyal suggested changes Dec 21, 2021
View reviewed changes
Copy link
Contributor

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

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

Not looking at all the Dart changes, its just too much.

modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java Show resolved Hide resolved
noordawod
noordawod approved these changes Dec 21, 2021
View reviewed changes
Copy link
Contributor

@noordawod noordawod left a comment

Choose a reason for hiding this comment

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

Amazing work 🤩

@noordawod
Copy link
Contributor

Not looking at all the Dart changes, its just too much.

Yeah, it took almost a day's work to painstakinly test, retest and reshape the changes, hopefully with amicable results for the public. But for me it still works in real-life apps and now it's null safe.

sbu-WBT
sbu-WBT approved these changes Dec 21, 2021
View reviewed changes
Copy link
Contributor

@sbu-WBT sbu-WBT left a comment

Choose a reason for hiding this comment

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

LGTM

@agilob
Copy link
Contributor Author

agilob commented Dec 21, 2021 •
edited
Loading

@wing328 can we have this merged to 6.x please! 🎉

@agilob
Copy link
Contributor Author

agilob commented Jan 2, 2022

@wing328 pls 👀

@wing328 wing328 merged commit 6c3cdee into OpenAPITools:6.0.x Jan 2, 2022
@wing328
Copy link
Member

wing328 commented Jan 2, 2022

Just merged. Sorry for the delay.

@wing328 wing328 added this to the 6.0.0 milestone Jan 2, 2022
@agilob
Copy link
Contributor Author

agilob commented Jan 3, 2022

well done everyone :)

@agilob agilob deleted the dart-2.12 branch January 3, 2022 09:20
@noordawod
Copy link
Contributor

Is there an estimate when v6 will be merged to master?

@agilob
Copy link
Contributor Author

agilob commented Jan 3, 2022

This month most likely, with 6.0 release

@agilob agilob mentioned this pull request Jan 9, 2022
Closed
@fatihozdil
Copy link

auto generated readme points to use the get method(apiVVersionPersonnelGet) but this method does not return anything
image

image

@AllenKaplan AllenKaplan mentioned this pull request Jul 15, 2022
Closed
eseidel
eseidel reviewed May 13, 2023
View reviewed changes
modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache
{{/isMap}}
{{^isMap}}
{{#isNumber}}
{{{name}}}: json[r'{{{baseName}}}'] == null
? null
: {{{datatypeWithEnum}}}.parse(json[r'{{{baseName}}}'].toString()),
? {{#defaultValue}}{{{.}}}{{/defaultValue}}{{^defaultValue}}null{{/defaultValue}}
Copy link

Choose a reason for hiding this comment

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

As far as I can tell, this line isn't quite right. It doesn't handle required num types, which will be (correctly) generated as non-nullable, but then this line tries to set them to null when they don't have a default value which won't compile.

https://raw.githubusercontent.com/SpaceTradersAPI/api-docs/b52734a2cdb63e796174310ebac89f5009df3b9f/reference/SpaceTraders.json

is an example of a spec which exhibits this, specifically:
https://raw.githubusercontent.com/SpaceTradersAPI/api-docs/b52734a2cdb63e796174310ebac89f5009df3b9f/models/JumpGate.json

Copy link
Contributor

Choose a reason for hiding this comment

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

@eseidel can you post this in a separate issue?
After these 2 PRs #14346 , #15477 are merged
we can now unify all efforts into one generator, which means solving a lot of bugs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmednfwela ahmednfwela ahmednfwela approved these changes

@Mike278 Mike278 Mike278 left review comments

@SimonIT SimonIT SimonIT left review comments

@kuhnroyal kuhnroyal kuhnroyal requested changes

@eseidel eseidel eseidel left review comments

@sbu-WBT sbu-WBT sbu-WBT approved these changes

@noordawod noordawod noordawod approved these changes

@jimschubert jimschubert Awaiting requested review from jimschubert

Assignees
No one assigned
Labels
Breaking change (without fallback) Client: Dart Enhancement: General
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.