OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
20.56k stars 6.28k forks source link

Update HttpRequest.cpp.mustache - use stable 4-parameter connect #18893

Open Jazzco opened 3 weeks ago

Jazzco commented 3 weeks ago

The 3-parameter connect is known to cause issues when the creating instance is deleted. In all other mustache you already use stable 4-parameter connect. In HttpRequest.cpp.mustache the fix was missing.

PR checklist

wing328 commented 3 weeks ago

thanks for the PR. please follow step 3 to update the samples.

cc @ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @martindelille (2018/03) @muttleyxd (2019/08)

Jazzco commented 3 weeks ago

I ran the parts of step 3, committed and pushed. Put I don't see a hint that these changes came in. Any hint what might went wrong?

Jazzco commented 3 weeks ago

Ah wait - I pushed to my master .... need to push to patch-1

Jazzco commented 3 weeks ago

Reviewing the changes I see a lot of path separator changes, obviously because I ran mvn and the scripts on Windows. Is that really what you want?

wing328 commented 2 weeks ago

@muttleyxd @MartinDelille when you guys have time, can you please PM me via Slack? Thanks.

Jazzco commented 2 weeks ago

Yes please push only the change regarding HttpRequest please!

As far as I see there is nothing changed but the manual change in HttpRequest, as long as the shifted entries of ApiException.php and readme.MD are irrelevant.

From a Windows user perspective the script I was asked to follow in step 3 is unusable in this state. Maybe it should be extended to replace all CR/LF by single LF.

I don't have the time to dig into it but the git bash seems to have commands for this:

find . -name "*.java" -exec dos2unix {} \;
wing328 commented 2 weeks ago

@Jazzco tests with updated samples failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9527512386/job/26264196606?pr=18939

can you please review the test failure when you've time?

Jazzco commented 2 weeks ago

@Jazzco tests with updated samples failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9527512386/job/26264196606?pr=18939

can you please review the test failure when you've time?

@wing328 I reviewed the change and don't see what could cause the issue. As mentioned above, step 3 produces many line break changes when processed on Windows, so I reverted it. Maybe I oversaw a generated change that was necessary but I guess it makes way more sense to run this script on Linux in the current state.

Locally generating our api and patching the results with these two changes afterwards works fine.

@ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @martindelille (2018/03) @muttleyxd (2019/08) Could one of you please take over this tiny change? And maybe trigger your scripting team to fix the linefeed-issue

Regards Jazzco

MartinDelille commented 2 weeks ago

@muttleyxd @MartinDelille when you guys have time, can you please PM me via Slack? Thanks.

I sent you an email!

Jazzco commented 2 weeks ago

I noticed that there were some other [clazy-connect-3arg-lambda] warnings, this time in api-body, and fixed them, too.

Jazzco commented 2 weeks ago

To get this right I ran step 3 from a Linux vm and pushed the generated changes now.

Jazzco commented 2 weeks ago

I tested these changes using Qt 5.15.2 on Windows. There were no compilation or link errors.

Jazzco commented 2 weeks ago

There were () missing and it obviously worked with the 3 param version but failed with the 4 param version under macOS. Hopefully it builds now.

wing328 commented 2 weeks ago

the CI tests failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9567139672/job/26402928612?pr=18893

please take a look when you've time.

Jazzco commented 2 weeks ago

I'm out of ideas. The linker error is way to universal to track it down to a cause.

clang: error: linker command failed with exit code 1 (use -v to see invocation)

Maybe if you were able to activate the -v option...

wing328 commented 2 weeks ago

Maybe if you were able to activate the -v option...

please update the workflow file to activate that

.github/workflows/samples-cpp-qt-client.yaml 
MartinDelille commented 1 week ago

I tried to reproduce the issue locally on my mac using Qt 5.15.17 without issue.

Jazzco commented 6 days ago

@MartinDelille The suggestions compile locally so I committed them.

MartinDelille commented 5 days ago

@wing328 any idea why the CI doesn't trigger properly ?

wing328 commented 5 days ago

triggered the workflow but some tests failed. please review when you guys have time

MartinDelille commented 1 day ago

I create a draft PR to troubleshoot if the problem comes from the Qt installation in the CI: #19049

MartinDelille commented 21 hours ago

The issue is not related to this PR changed. It should be fixed after #19049 is merged.