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

upgrade dart generator dependencies #9392

Merged
merged 10 commits into from
May 10, 2021
Merged

upgrade dart generator dependencies #9392

merged 10 commits into from
May 10, 2021

Conversation

Adi1231234
Copy link
Contributor

upgrade dart generator dependencies

@wing328
Copy link
Member

wing328 commented May 4, 2021

cc @swipesight (2018/09) @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12)

kuhnroyal
kuhnroyal suggested changes May 4, 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.

I want to leave #8978 (comment) for consideration.
@agilob I am not sure if json_serializable is still involved but as you noted previously, this will most likely be a breaking change.

modules/openapi-generator/src/main/resources/dart2/pubspec.mustache Outdated Show resolved Hide resolved
modules/openapi-generator/src/main/resources/dart2/pubspec.mustache Outdated Show resolved Hide resolved
@kuhnroyal
Copy link
Contributor

We basically had the same discussion in #8978 - the question that remains is: Can we live with a breaking change (due to Dart 2.12.0 requirement) or do we create a new generator like I did with the dart-dio-next generator?
The json_serializable option is basically still experimental, if our only breaking changes are there, then we can go for it imo.

@kuhnroyal kuhnroyal mentioned this pull request May 4, 2021
Closed
6 tasks
@agilob
Copy link
Contributor

agilob commented May 4, 2021 •
edited
Loading

The json_serializable option is basically still experimental, if our only breaking changes are there, then we can go for it imo.

agreed. breaking change in this generator will be new field in an annotation, one line change to fix, but it will require D2.12

@agilob
Copy link
Contributor

agilob commented May 4, 2021

Is this going to break a lot of other things? the whole nnbd, is it going to even compile with 2.12?

@kuhnroyal
Copy link
Contributor

Well, you can use Dart 2.12 without using NNBD. These changes are for now just Dart 2.12.
But json_serializable seems to be using the Dart type to determine nullability, this would require NNBD to be working.
Maybe null handling is now broken for the json_serializable option, not completely sure.

@kuhnroyal
Copy link
Contributor

This is starting to look like it may work, nice work @Adi1231234!

@agilob The tests for petstore_json_serializable_client_lib_fake are currently disabled but that is cause they don't compile fully anyways right?

<!--<module>samples/openapi3/client/petstore/dart2/petstore_json_serializable_client_lib_fake</module>-->

All the other samples are compiling. That sadly doesn't mean everything will work at runtime :) We need some people to test this.

@agilob
Copy link
Contributor

agilob commented May 4, 2021

@agilob The tests for petstore_json_serializable_client_lib_fake are currently disabled but that is cause they don't compile fully anyways right?

yea, never managed to enable them as that generator was never finalized.

kuhnroyal
kuhnroyal reviewed May 4, 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.

Ok, in the current state, I believe this may work but I have not tested it. I suggest to not merge this before the 5.1.1 release. I think this should go into 5.2.0

@Adi1231234
Copy link
Contributor Author

Ok, in the current state, I believe this may work but I have not tested it. I suggest to not merge this before the 5.1.1 release. I think this should go into 5.2.0

Do I have to do anything else to get it into 5.2.0?

@kuhnroyal
Copy link
Contributor

Ok, in the current state, I believe this may work but I have not tested it. I suggest to not merge this before the 5.1.1 release. I think this should go into 5.2.0

Do I have to do anything else to get it into 5.2.0?

I think you can just switch the target branch to 5.2.x and then it can be merged, might have to rebase.

@Adi1231234 Adi1231234 changed the base branch from master to 5.2.x May 5, 2021 17:21
@Adi1231234
Copy link
Contributor Author

Ok, in the current state, I believe this may work but I have not tested it. I suggest to not merge this before the 5.1.1 release. I think this should go into 5.2.0

Do I have to do anything else to get it into 5.2.0?

I think you can just switch the target branch to 5.2.x and then it can be merged, might have to rebase.

Thanks, I just did it, I would be happy if you could approve it

kuhnroyal
kuhnroyal approved these changes May 6, 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.

LGTM, was waiting with approve only so that @wing328 doesn't merge it into master.
Thanks for your work on this!

@agilob
Copy link
Contributor

agilob commented May 6, 2021

I'm guessing it's time to upgrade json_serializable after this gets in

@kuhnroyal
Copy link
Contributor

Yea, and NNBD :)

@Adi1231234
Copy link
Contributor Author

Hi, I do not know so much how merges works here. but I noticed that the branch has not been marged.
I wanted to ask, should I do something else or talk to someone in order the branch to be merge?

@agilob
Copy link
Contributor

agilob commented May 9, 2021

this PR will be closed after it's merged and you will receive a notification

@kuhnroyal
Copy link
Contributor

I am sure @wing328 was busy with the 5.1.1 release. This will be merged in the next few days, no worries.

@wing328 wing328 added this to the 5.2.0 milestone May 10, 2021
@wing328 wing328 changed the base branch from 5.2.x to master May 10, 2021 07:58
@wing328 wing328 merged commit 1961c66 into OpenAPITools:master May 10, 2021
@wing328
Copy link
Member

wing328 commented May 10, 2021

Thanks for the PR, which has been merged into master (upcoming 5.2.0 release)

@wing328 wing328 mentioned this pull request May 10, 2021
Closed
5 tasks
@kuhnroyal kuhnroyal mentioned this pull request May 18, 2021
Merged
5 tasks
kuhnroyal added a commit to kuhnroyal/openapi-generator that referenced this pull request May 18, 2021
@kuhnroyal
[dart] Fix petstore sample tests & Multipart not working
d4d29ea 
* follow up to OpenAPITools#9392
* `MultipartRequest(null, null)` is no longer valid after `http` package upgrade
* fix petstore sample project not compiling
* disable live petstore tests
* use template to strip out unused code blocks
wing328 pushed a commit that referenced this pull request May 18, 2021
@kuhnroyal
Fix CircleCI failures (#9503)
55b95bc 
* Fix go-echo-server sample not running in CircleCI

* remove wrong argument

* [dart] Fix petstore sample tests & Multipart not working

* follow up to #9392
* `MultipartRequest(null, null)` is no longer valid after `http` package upgrade
* fix petstore sample project not compiling
* disable live petstore tests
* use template to strip out unused code blocks
premiumitsolution pushed a commit to premiumitsolution/openapi-generator that referenced this pull request May 26, 2021
@kuhnroyal @Fornori
Fix CircleCI failures (OpenAPITools#9503)
1ba6e4d 
* Fix go-echo-server sample not running in CircleCI

* remove wrong argument

* [dart] Fix petstore sample tests & Multipart not working

* follow up to OpenAPITools#9392
* `MultipartRequest(null, null)` is no longer valid after `http` package upgrade
* fix petstore sample project not compiling
* disable live petstore tests
* use template to strip out unused code blocks
@kuhnroyal kuhnroyal mentioned this pull request May 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaumard jaumard jaumard left review comments

@kuhnroyal kuhnroyal kuhnroyal approved these changes

@agilob agilob agilob approved these changes

@jimschubert jimschubert Awaiting requested review from jimschubert

Assignees
No one assigned
Labels
Client: Dart Enhancement: Compatibility
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Adi1231234 @wing328 @kuhnroyal @agilob @jaumard