[C++] [Qt5] Clang format Code#4444
Conversation
|
This is a great PR. Possible values:
Dump cc |
|
I've had to cancel this PR's CircleCI build because it was blocking our queue for 6 hours. Once the discussion above about style is addressed, please trigger CircleCI again. |
|
I open to any suggestion concerning the style. I think I used the |
There was a problem hiding this comment.
default LLVM is All
There was a problem hiding this comment.
It allowed me to if statement with many condition into several lines.
There was a problem hiding this comment.
The main issue I was dealing with this setting is that if let to default, the description was splitted into several lines which is not easy to do in the mustache template.
There was a problem hiding this comment.
@MartinDelille Are we going to restore .clang-foramt to the default?
There was a problem hiding this comment.
I could remove this file since it is not really used.
I used it to help me to generate a better output but was unable to respect the specific ColumnLimit rule, in particular when writing the header documentation.
The current template generates:
/**
* OpenAPI Petstore
* This is a sample server Petstore server. For this sample, you can use the api key `special-key` to test the authorization filters.
*
* The version of the OpenAPI document: 1.0.0
*
* NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
* https://openapi-generator.tech
* Do not edit the class manually.
*/
Setting ColumnLimit to the default would require the following generation:
/**
* OpenAPI Petstore
* This is a sample server Petstore server. For this sample, you can use the api
* key `special-key` to test the authorization filters.
*
* The version of the OpenAPI document: 1.0.0
*
* NOTE: This class is auto generated by OpenAPI Generator
* (https://openapi-generator.tech). https://openapi-generator.tech Do not edit
* the class manually.
*/
I have no idea how to split the description from the mustache template.
There was a problem hiding this comment.
I have no idea how to split the description from the mustache template.
My take is to leverage code formatter to take care of the line split.
There was a problem hiding this comment.
Yes I got it! This .clang-format will only be used if we want to check the template generate a clang format compatible code.
|
My suggestion is to include a default .clang-format (auto-generated) and let the user customize it to meet their requirements/preferences. |
|
@wing328 This could make the templating easier to write but wouldn't it require to have clang-format as a dependency? |
To be clear, we don't need to update the template. We simply leverage code formatter to do the job for us. Users can use file post-processing (as part of openapi-generator) or use the code formatter on the output after the code generation completes. Using Ruby as an example, the generator outputs the |
|
@wing328 I totally agree with you about not modifying the template: tweaking it to respect the rules I used for the example was not an easy task. I will look into the ruby generator to mimic the behavior. |
|
@etherealjoy Regarding the style, we should maybe have |
Agreed with using clang-format's default style as a starting point. |
|
After investigating I realised that the processing already work and is even more generic than We just need to set The question is: do we want to enable post processing for the sample? |
We'd a discussion on that before (we also tried) but the problem is not all contributors have the code formatter installed. |
|
Yes that is what I was thinking. Do you think improving the style of the generated code is still relevant? |
|
@MartinDelille I definitely like your change. A little background on We still try to make the default output looks good in terms of code format so we definitely welcome your PR to update the template with better code format. Please resolve the merge conflicts and I'll merge this PR. |
MartinDelille
force-pushed
the
clang-format
branch
from
2e77b3f to
812ad2e
Compare
|
Ok I rebased my PR against #4526 which is more important to me (having |
|
Please consider merging this PR (and #4526) before other cpp-qt5 template modification because rebasing and merging conflict can be quite long! 😅 |
|
Just one additional comment, rest looks fine. |
MartinDelille
force-pushed
the
clang-format
branch
from
2cdf238 to
a84a003
Compare
|
I used |
|
It will be squashed on merge so not necessary. |
|
Is something blocking this PR to be merged? If someone else is working on the qt5 client template, there is a risk of ruining the job done here. |
4 participants
This is an attempt to set several clang-format rules on the generated code.
What do you think about it?
PR checklist
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.@ravinikam @stkrwork @etherealjoy @muttleyxd