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] Sanitize identifier names #631

Conversation

Jauler
Copy link
Contributor

@Jauler Jauler commented Jul 23, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Proposed fix for #621. Add cpp identifier sanitizing to AbstractCppCodegen.java and provide those functions for other cpp codegens if needed.

NOTE: After adding to AbstractCppCodegen.java it is possible to remove some overriden functions from other (than cpprestsdk) cpp generators, but I wanted to avoid that as I have no setup for testing those.

@ravinikam (2017/07) @stkrwork (2017/07) @fvarose (2017/11) @etherealjoy (2018/02) @MartinDelille (2018/03)

@wing328
Copy link
Member

wing328 commented Jul 24, 2018

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@Jauler Jauler force-pushed the cpprestsdk_invalid_characters_in_identifiers branch from 47e4471 to f2694d6 Compare July 24, 2018 08:25
@Jauler
Copy link
Contributor Author

Jauler commented Jul 24, 2018

Fixed :)

@etherealjoy
Copy link
Contributor

@Jauler
Do you wish to clean up duplicate code in the sub classes?

@Jauler
Copy link
Contributor Author

Jauler commented Jul 25, 2018

Hey,
While I was cleaning up duplicated code, I had some observations:

  • CppTizen had already implemented some sanitizing, although It simply deleted invalid characters and sanitizeNames() replaces them with "_". I changed CppTizen to use default behaviour.
  • CppQt on toApiName and toModelName methods prepended modelNamePrefix to the name. I believe this is good fit for default behaviour, therefore I added that to AbstractCppCodegen.
  • CppQt in toParamName and toVarName converts to snake_case, does not allow all caps etc. I was not sure why it is needed, so I left this behaviour untouched.

@Jauler Jauler force-pushed the cpprestsdk_invalid_characters_in_identifiers branch from 7b71a23 to 264a864 Compare July 25, 2018 21:02
@wing328
Copy link
Member

wing328 commented Jul 26, 2018

@Jauler thanks for cleaning up the duplicated code. Please also run the shell script (./bin/) or Windows batch files (.\bin\windows) to update the Petstore samples for the affected generators (e.g. Tizen) to see if there're any changes to the samples as a result of the cleanup.

@Jauler
Copy link
Contributor Author

Jauler commented Jul 26, 2018

Good Catch!
I missed the fact, that tizen codegen is not extending AbstractCppCodegen class which caused some unintended differences in code generation.

I believe that making tizen extend AbstractCppCodegen is better suited for separate PR and adding that I have no tizen setup, I would like to be on the safe side, therefore I simply reverted changes done to tizen.

@wing328 wing328 merged commit 0b88889 into OpenAPITools:master Jul 28, 2018
@wing328
Copy link
Member

wing328 commented Jul 28, 2018

@Jauler PR merged into master. Thanks for your contribution.

I've created #674 to track extending TizenCodegen with AbstractCppCodegen

@etherealjoy
Copy link
Contributor

I will also remove the override in the Qt5 Server subclass which is extending the AbstractCppCodegen.

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* [cpp] Sanitize identifier names

* Remove duplicated methods in cpp code generator subclasses.

* Fix unintended codegen differences in cpp tizen caused by it not extending AbstractCppCodegen class.
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.

None yet

4 participants