Skip to content

[BUG][C++] Avoid using plain underscore when escaping reserved words#5269

Merged
wing328 merged 5 commits intoOpenAPITools:masterfrom
alexweav:users/alweaver/escape-prefix
Feb 24, 2020
Merged

[BUG][C++] Avoid using plain underscore when escaping reserved words#5269
wing328 merged 5 commits intoOpenAPITools:masterfrom
alexweav:users/alweaver/escape-prefix

Conversation

@alexweav
Copy link
Copy Markdown
Contributor

@alexweav alexweav commented Feb 10, 2020 •
edited
Loading

Fixes #5268 as described in the issue. Are there any better preferred ways of escaping this?

Ran the generator against the file given in the issue, and verified that the output now compiled using MSVC-14. Repeated below, for ease of use:

    openapi: 3.0.0
    info:
      title: Inline API
      version: 0.0.1
    paths:
      /things:
        get:
          summary: Returns a list of things.
          parameters:
            - in: query
              name: inline
              schema:
                type: boolean
              description: Matches a keyword in C++.
          responses:
            '200':    # status code
              description: A JSON array of things.
              content:
                application/json:
                  schema: 
                    type: array
                    items: 
                      type: string

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@alexweav alexweav requested a review from etherealjoy February 10, 2020 21:25
@alexweav
Copy link
Copy Markdown
Contributor Author

alexweav commented Feb 10, 2020

cc @ravinikam @stkrwork @MartinDelille @muttleyxd

@wing328
Copy link
Copy Markdown
Member

wing328 commented Feb 11, 2020

@alexweav thanks for the PR.

What about adding an option reservedWordPrefix so that people can customize it in whatever way they want (and the change will be backward compatible)?

We can default it to r_ like what you've suggested.

@alexweav
Copy link
Copy Markdown
Contributor Author

alexweav commented Feb 21, 2020

@wing328 @etherealjoy Apologies for the slow response on this. I added a CLI option, reservedWordPrefix, which allows users to configure the prefix, although it will still default to r_.

I made this change in the base abstract generator for C++, but I noticed that the CppRestSdkClientCodegen actually clears all CLI options inherited from its base class. Removing this clear-all brought in more unwanted options, so I instead opted for adding the option directly to the CppRest class too. Is there a better way of working around this?

Since this option was added to the base abstract generator, it will be inherited by all other C++ generators which do not clear their inherited options. I noticed that the CppPistacheServer generator and the CppRestbedServer generator also clear their inherited options, but the other C++ generators don't. If you would like, I can explicitly add this option to these two as well, following the same approach I took for the CppRest generator. However, if there is a different/more generic approach that you would prefer, please let me know.

@etherealjoy
Copy link
Copy Markdown
Contributor

etherealjoy commented Feb 22, 2020

LGTM.
However there is a CI issue because some of the changed files were not archived.
Could you please check in those files as well, they got changed due to this newly added flag

	modified:   docs/generators/cpp-qt5-client.md
	modified:   docs/generators/cpp-qt5-qhttpengine-server.md
	modified:   docs/generators/cpp-restsdk.md
	modified:   docs/generators/cpp-tizen.md

I was going to suggest the reserved keyword prefix be left as _ but i realized it will not compile on Windows MSVC.

@wing328 wing328 added this to the 4.3.0 milestone Feb 24, 2020
@wing328 wing328 merged commit 231ec6b into OpenAPITools:master Feb 24, 2020
@wing328
Copy link
Copy Markdown
Member

wing328 commented Feb 24, 2020

If you would like, I can explicitly add this option to these two as well, following the same approach I took for the CppRest generator. However, if there is a different/more generic approach that you would prefer, please let me know.

Please file another PR with the same approach to do so. Thank you.

@alexweav alexweav mentioned this pull request Feb 24, 2020
Merged
5 tasks
@alexweav
Copy link
Copy Markdown
Contributor Author

alexweav commented Feb 24, 2020

Please file another PR with the same approach to do so. Thank you.

Posted: #5418

@muttleyxd muttleyxd mentioned this pull request Feb 25, 2020
Merged
5 tasks
MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
@alexweav @MikailBag
[BUG][C++] Avoid using plain underscore when escaping reserved words (O…
ee63482 
…penAPITools#5269)

* Don't use plain underscore when escaping reserved words

* Regenerate petstore output

* Add CLI option for reserved word prefix

* Ensure CLI option isn't cleared in cpprest client codegen

* Regenerate docs
@wing328
Copy link
Copy Markdown
Member

wing328 commented Mar 27, 2020

@alexweav thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952

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

Reviewers

@etherealjoy etherealjoy etherealjoy approved these changes

+1 more reviewer

@muttleyxd muttleyxd muttleyxd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Client: C++ Enhancement: Feature Issue: Bug Server: C++

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

[BUG][C++] Reserved keywords are escaped via underscore prefix, resulting in compile failure on MSVC 14

4 participants

@alexweav @wing328 @etherealjoy @muttleyxd