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

[cpp-qt-client] New makeOperationsVirtual option #19613

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

limentas
Copy link
Contributor

@limentas limentas commented Sep 18, 2024

This PR implements new makeOperationsVirtual option for cpp-qt-client generator. If this option has true value then generated operations become virtual. This allows to easily mock up generated code and unit test the code which uses generated classes. You can find example of generated code here as a diff. On the left side the code how it looks like before the changes or with default value of the new option. And on the right side the code how it looks like after the changes and with the option set to true.

Fix #19512

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/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    @ravinikam @stkrwork @etherealjoy @MartinDelille @muttleyxd

@limentas limentas marked this pull request as ready for review September 18, 2024 12:00
Copy link
Contributor

@muttleyxd muttleyxd left a comment

Choose a reason for hiding this comment

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

Looks good, I wonder if it shouldn't be set to true by default. Both ways are fine for me though.

@limentas
Copy link
Contributor Author

Thanks for your review.
Talking about making the option true by default. My point was to not change current behavior. But I don't see any harm if it is on by default. Since *Api class already has QObject as a parent it already has virtual table. I also don't see a code that can be broken with this change as well. But maybe I'm missing something.

@MartinDelille
Copy link
Contributor

Why not just adding the virtual keyword and without the option ?

@limentas
Copy link
Contributor Author

My intention is to not break something. Although I don't foreseen problems I may miss something. If you guys think that we can make this behavior default. I'm all for it.

@wing328
Copy link
Member

wing328 commented Sep 22, 2024

My intention is to not break something. Although I don't foreseen problems I may miss something. If you guys think that we can make this behavior default. I'm all for it.

Thanks for taking consideration into backward compatibility.

Upcoming release allows breaking changes with fallbacks so please set the option to true by default and we will see if any user encounters issues with this enhancement.

@limentas
Copy link
Contributor Author

@wing328 Thank you for your input. This totally makes sense.
I've made the new parameter true by default.

@wing328
Copy link
Member

wing328 commented Sep 23, 2024

can you please update the samples (step 3) as well as we should see some changes in the c++ qt petstore client?

@limentas
Copy link
Contributor Author

limentas commented Sep 23, 2024

can you please update the samples (step 3) as well as we should see some changes in the c++ qt petstore client?

@wing328 Thanks for reminder. It turned out I've made the property true by default in a wrong way. I figured out and fixed the issues.

@wing328 wing328 merged commit 7d8eacc into OpenAPITools:master Sep 23, 2024
21 checks passed
@MartinDelille
Copy link
Contributor

Happy to see new contributor to the qt cpp generator! 🎉

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.

[REQ] [cpp-qt-client] Make generated methods optionally virtual
4 participants